Make backward compliance for the workspace keys with ":" and optional namespace value #4123

benoitf · 2017-02-13T11:18:16Z

What does this PR do?

PR #4073 has broken any client using existing workspace keys
Restore the handling of previous keys format

What issues does this PR fix or reference?

#4061

Changelog

Restore backward compliance broken on workspace keys by #4073
so clients don't need to be updated

Release Notes

Restore backward compliance broken on workspace keys #4073

Docs PR

N/A

Change-Id: I884e8adef2f01774f00a9f6c08ca834fc22b5917
Signed-off-by: Florent BENOIT [email protected]

… namespace value Broken by #4061 Change-Id: I884e8adef2f01774f00a9f6c08ca834fc22b5917 Signed-off-by: Florent BENOIT <[email protected]>

gazarenkov · 2017-02-13T13:01:59Z

I would not leave old format until we have really serious reason for that.
Do we know some third-party clients sensitive to this change?
Otherwise, the sooner we change it - the better.

Instead I'd add add exception to the service like this:
if(key.indexOf(":"))
throw new ServerException("Since version ... the format ... is not supported. Use ... instead");

benoitf · 2017-02-13T13:04:40Z

@gazarenkov : Why is it a problem to get backward compliance ?
All links are using new formats, it's just for ppl retrieving in the previous format, it's still working.

Workspace KEY is something of the core part and not in a sub-sub part of plug-In so it's something that you should not break. And even more when backward compliance is possible

gazarenkov · 2017-02-13T13:15:12Z

Sure, it requires changes and yes, may be not pleasant, it is obvious.
But need to estimate what is actually cheaper for us for now and for future. since it requires maintaining API options which we do not plan to support in future.
So, I need estimation of real risk, as I asked, before we make a decision.

benoitf · 2017-02-13T13:24:53Z

@gazarenkov risk is high (and of course you can't estimate because by definition, with a public API you don't maintain all clients) as it will be introduced for a version without deprecation time or introducing the new format.
Also you can have clients (like CLI) that will break usage of ppl given their workspace name in their usual format. (users won't be happy) (side note : also I don't see any reason to force ppl to give a namespace when it belongs directly to their account. It requires extra typing effort and I see it as a regression).
Also by definition a client is not colocated so you can have the same client connecting to different systems. And there is no way to know the level of the API on each system (because it returns today the same version since like a couple of years)

For real estimate and better result, you need to first deprecate when introducing a new format, keep things for a while, then remove deprecate. Not : replace directly.

gazarenkov · 2017-02-13T14:37:25Z

What you said is pretty obvious :). What I said is I would not blindly follow procedure you mentioned , I need to estimate real risk to not to be the hostage of API which is opposite to the concept.

So, I have not thrown away any option yet, I asked if you know real clients other than ones we know about which may be impacted.

Why not to consider making CLI backward compatible to the old syntax if it is some important to you BTW?

benoitf · 2017-02-13T14:48:30Z

here we derivate from the original issue that is currently, the CLI (and maybe other clients) are broken due to breaking changes, while at any time, code should be shippable and this PR fix the current state.

about the future of cli:

Why not to consider making CLI backward compatible to the old syntax if it is some important to you BTW?

I would say that it's better when all clients are still compliant by doing nothing on their side than having to update each clients ? Because the effort is pretty easy on the server side.
BTW, how the client could know what the server side support as format as the API level is the same before and after the change while API is not working in the same way ?

I'm not arguing BTW that API should never be changed. Sometimes it requires changes but it's just that when changes are immediate, it breaks and it can be avoided by having timeframe and deprecation time for clients to have time to update.
So by having some period of time, CLI could be updated but it's not planned for the current sprint while API is broken right now.
And yes, I'm still discussing about API versioning, API level so that clients can work smoothly.

so yes, why not updating clients but:

Server side in this case can handle backward compliance easily
A correct API level on server side will help clients to address all versions of APIs (here it fails)
A versioning of the API on server side will allow timeframe for clients to update while server side code is cleaner for the current latest version. (And then it allows the server side to drop old support after a period of some releases)

codenvy-ci · 2017-02-13T17:16:55Z

Build success. https://ci.codenvycorp.com/job/che-pullrequests-build/1956/

sleshchenko · 2017-02-15T08:46:15Z

...-core-api-workspace/src/main/java/org/eclipse/che/api/workspace/server/WorkspaceManager.java

@@ -895,13 +895,27 @@ private WorkspaceImpl doCreateWorkspace(WorkspaceConfig config,
    }

    private WorkspaceImpl getByKey(String key) throws NotFoundException, ServerException {
+
+        int lastColonIndex = key.indexOf(":");


Should we fix doc for WorkspaceManager#getWorkspace https://github.com/eclipse/che/pull/4073/files#diff-349ffaa889025cd6acd2d5b512c2e687L183 ?

@sleshchenko that's a good question.
I will add info with deprecated marker

@sleshchenko 07cd05d
is it ok ?

…ptional namespace value Broken by #4061

sleshchenko · 2017-02-15T09:16:46Z

...-core-api-workspace/src/main/java/org/eclipse/che/api/workspace/server/WorkspaceManager.java

@@ -179,6 +179,8 @@ public WorkspaceImpl createWorkspace(WorkspaceConfig config,
     *
     * <p> Key rules:
     * <ul>
+     * <li>@Deprecated : If it contains <b>:</b> character then that key is combination of namespace and workspace name
+     * <li>@Deprecated : <b></>:workspace_name</b> is valid abstract key and user will be detected from Environment.


I'd rewrite to something like and current user name will be used as namespace

ok I've used previous javadoc
now I've updated with your remark

…ptional namespace value Broken by #4061

codenvy-ci · 2017-02-15T10:58:23Z

Build # 1976 - FAILED

Please check console output at https://ci.codenvycorp.com/job/che-pullrequests-build/1976/ to view the results.

… namespace value (eclipse-che#4123) * Make backward compliance for the workspace keys with ":" and optional namespace value Broken by eclipse-che#4061 This backward compliance will be soon removed once deprecation time is over Change-Id: I884e8adef2f01774f00a9f6c08ca834fc22b5917 Signed-off-by: Florent BENOIT <[email protected]>

Make backward compliance for the workspace keys with ":" and optional…

9bf41b1

… namespace value Broken by #4061 Change-Id: I884e8adef2f01774f00a9f6c08ca834fc22b5917 Signed-off-by: Florent BENOIT <[email protected]>

benoitf added the kind/bug Outline of a bug - must adhere to the bug report template. label Feb 13, 2017

benoitf self-assigned this Feb 13, 2017

benoitf requested review from ashumilova and sleshchenko February 13, 2017 11:18

benoitf added the severity/blocker Causes system to crash and be non-recoverable or prevents Che developers from working on Che code. label Feb 14, 2017

benoitf added this to the 5.3.0 milestone Feb 14, 2017

benoitf added the status/code-review This issue has a pull request posted for it and is awaiting code review completion by the community. label Feb 14, 2017

ashumilova approved these changes Feb 15, 2017

View reviewed changes

sleshchenko reviewed Feb 15, 2017

View reviewed changes

sleshchenko approved these changes Feb 15, 2017

View reviewed changes

fixup! Make backward compliance for the workspace keys with ":" and o…

07cd05d

…ptional namespace value Broken by #4061

sleshchenko reviewed Feb 15, 2017

View reviewed changes

fixup! Make backward compliance for the workspace keys with ":" and o…

dd0425a

…ptional namespace value Broken by #4061

benoitf requested a review from slemeur February 15, 2017 09:19

slemeur approved these changes Feb 15, 2017

View reviewed changes

benoitf removed the status/code-review This issue has a pull request posted for it and is awaiting code review completion by the community. label Feb 15, 2017

benoitf merged commit a1d230c into master Feb 15, 2017

benoitf deleted the backward-compliant-workspace-key branch February 15, 2017 10:22

JamesDrummond mentioned this pull request Feb 17, 2017

5.3.0 Milestone Overview #4030

Closed

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make backward compliance for the workspace keys with ":" and optional namespace value #4123

Make backward compliance for the workspace keys with ":" and optional namespace value #4123

benoitf commented Feb 13, 2017

gazarenkov commented Feb 13, 2017

benoitf commented Feb 13, 2017

gazarenkov commented Feb 13, 2017

benoitf commented Feb 13, 2017

gazarenkov commented Feb 13, 2017

benoitf commented Feb 13, 2017 •

edited

Loading

codenvy-ci commented Feb 13, 2017

sleshchenko Feb 15, 2017

benoitf Feb 15, 2017

benoitf Feb 15, 2017

sleshchenko Feb 15, 2017

benoitf Feb 15, 2017

codenvy-ci commented Feb 15, 2017

Make backward compliance for the workspace keys with ":" and optional namespace value #4123

Make backward compliance for the workspace keys with ":" and optional namespace value #4123

Conversation

benoitf commented Feb 13, 2017

What does this PR do?

What issues does this PR fix or reference?

Changelog

Release Notes

Docs PR

gazarenkov commented Feb 13, 2017

benoitf commented Feb 13, 2017

gazarenkov commented Feb 13, 2017

benoitf commented Feb 13, 2017

gazarenkov commented Feb 13, 2017

benoitf commented Feb 13, 2017 • edited Loading

codenvy-ci commented Feb 13, 2017

sleshchenko Feb 15, 2017

Choose a reason for hiding this comment

benoitf Feb 15, 2017

Choose a reason for hiding this comment

benoitf Feb 15, 2017

Choose a reason for hiding this comment

sleshchenko Feb 15, 2017

Choose a reason for hiding this comment

benoitf Feb 15, 2017

Choose a reason for hiding this comment

codenvy-ci commented Feb 15, 2017

benoitf commented Feb 13, 2017 •

edited

Loading