Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

#55: Naming convention changes #111

Merged
merged 10 commits into from
Apr 24, 2019
Merged

Conversation

garagatyi
Copy link

@garagatyi garagatyi commented Apr 17, 2019

What does this PR do?

Changes:

  • Rename field name to displayName
  • Rename field id to name
  • Add v2 folder in root of registry. To use new notation registry URL needs to be changed to old_url/v2/
  • Copy plugins into publishers folders under /v2/ (but leave them in old location for backward compatibility)
  • Remove /meta.yaml from plugin links in index to allow avoiding implicit addition of /meta.yaml to paths to meta files on downloading
  • Add deprecate section to old plugins
  • Refactor scripts
  • Add migrate section to registry index
  • Add generated id field with publisher/name/version as a value to index
  • Set publisher to plugins in new locations according to what we have discussed in Naming convention #55
  • Set empty publisher in old plugins locations
  • Remove dummy plugins with new notation under /v2/

Related to #55

@garagatyi
Copy link
Author

@skabashnyuk @mkuznyetsov Please, review scripts - I changed that significantly.
@l0rd Please, review new plugins publisher and name

@garagatyi
Copy link
Author

@amisevsk @davidfestal @ibuziuk FYI

@l0rd
Copy link
Contributor

l0rd commented Apr 17, 2019

@garagatyi please move the new plugins in a v2 folder (i.e. v2/plugins/<publisher>/<name>/<version>. Having old and new plugins in the same folder is kind of awkward. I don't want you to create a v1 folder. We can leave the old plugins in the root folder but at least the v2 folder will be clean and if we will continue using this pattern if when we introduce breaking changes (v3, v4 etc...)

You should remove examples and dummy plugins as well (all in the ws-skeleton folder except the 2 editors).

README.md Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
@garagatyi
Copy link
Author

@l0rd I moved plugins with new notation to v2 folder.
To use old registry we should leave URL to the registry as is. This ensures that workspaces in older Che will work after the update. id in old index.json is equal to the name for backward compatibility.
To use new notation add /v2 to the URL of the registry. This is what we will do in Che after updating UD and WS master/broker. When v2 is used id in index.json contains publisher/name/version.
I also removed dummy plugins as you suggested.

@l0rd
Copy link
Contributor

l0rd commented Apr 18, 2019

@garagatyi good job!

@garagatyi
Copy link
Author

Related eclipse-che/che#12910

@garagatyi
Copy link
Author

@mkuznyetsov @skabashnyuk Please, review

@garagatyi
Copy link
Author

@l0rd If we remove dummy plugins existing workspaces with them, if any, would be broken. Is it OK for you?
Please, also review publisher/name in plugins to ensure there would not be any mistakes on my side.

@garagatyi
Copy link
Author

Take into account that old plugins we re also copied to v2 to avoid breaking existing workspaces that use old notation

@garagatyi
Copy link
Author

@l0rd I did not remove dummy plugins with old convention, only with new one. It did not break anything. Sorry for the misinformation

@garagatyi
Copy link
Author

@sleshchenko @ibuziuk I addressed your comments, please, take a look again

@garagatyi
Copy link
Author

It was difficult to properly rebase on top of master so I squashed all the commits. Sorry for the inconvenience.

Copy link
Member

@sleshchenko sleshchenko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left some comments and questions. Sorry if I was able to find answers for my question in PR description or in discussion but I did not.

README.md Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
Dockerfile Outdated Show resolved Hide resolved
plugins/redhat.vscode-openshift-connector/0.0.17/meta.yaml Outdated Show resolved Hide resolved
v2/plugins/che-machine-exec-plugin/0.0.1/meta.yaml Outdated Show resolved Hide resolved
v2/plugins/che-machine-exec-plugin/0.0.1/meta.yaml Outdated Show resolved Hide resolved
Copy link
Member

@sleshchenko sleshchenko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Oleksandr Garagatyi added 2 commits April 24, 2019 11:25
…cases for backward compatibility

Signed-off-by: Oleksandr Garagatyi <ogaragat@redhat.com>
Signed-off-by: Oleksandr Garagatyi <ogaragat@redhat.com>
Rename field name to displayName.
Rename field id to name.
Remove /meta.yaml from plugin links in index.
Add deprecate section to old plugins.
Rework scripts.
Add migrate section to registry index.
Add old notation plugins to v2 not to break exisiting workspaces.
Adapt Readme.md.

Signed-off-by: Oleksandr Garagatyi <ogaragat@redhat.com>
@garagatyi
Copy link
Author

FYI I reworked PR not to change anything in v1 plugins to ensure that everything will be backward compatible when OSIO registry will be updated

Signed-off-by: Oleksandr Garagatyi <ogaragat@redhat.com>
@l0rd
Copy link
Contributor

l0rd commented Apr 24, 2019

@garagatyi I still see a lot of plugins folders in the v2 that are not in the format <plublisher>/<name> or that are sample plugins. I suspect that this is for backward compatibility but it doesn't make sense to me: we are in beta and I haven't seen any real adoption of Che 7 workspaces yet (except for IDE2 team I suppose). An email to che-dev explaining how to fix the config of an existing workspace should be enough.

@garagatyi
Copy link
Author

@l0rd yes, those are for the backward compatibility.
I did think that backward compatibility is a concern. If it is not then we could simplify a lot of things.
@ibuziuk how do you think can we avoid adding backward compatibility for plugins notation for OSIO-Che?

@l0rd
Copy link
Contributor

l0rd commented Apr 24, 2019

@garagatyi @ibuziuk do you know any real user of Che 7 workspaces on osio-che? As a reminder Che 7 workspaces didn't worked there for a few months and stacks are really old ones.

@ibuziuk
Copy link
Member

ibuziuk commented Apr 24, 2019

@l0rd well, from the beginning, we were planning to have backward compatibility with existing workspaces on che.openshift.io. Are we sure that we opt for breaking things on the live service with just a che-dev email notification?

@l0rd
Copy link
Contributor

l0rd commented Apr 24, 2019

@ibuziuk my understanding is that Che 7 has been unusable on che-osio until 2 weeks ago. And that currently it's still kind of not easily usable. Hence I suspect that we don't have users and I would like to avoid that we spend time for non existing users. Backward compatibility makes sense only if we have users actively using Che 7 workspaces.

Signed-off-by: Oleksandr Garagatyi <ogaragat@redhat.com>
@garagatyi
Copy link
Author

@l0rd makes sense. My frustration comes from the fact that we didn't go to this direction initially.
@ibuziuk if you are OK with the plan I'll go with it

@ibuziuk
Copy link
Member

ibuziuk commented Apr 24, 2019

well, I'm ok - it is just that I also thought that the backward compatibility was the initial plan we discussed during prio calls

@garagatyi
Copy link
Author

@l0rd but we will still leave /plugins and /v2/plugins in the registry because prod-preview deploys registry on each commit, so I don't want to break at least this

Oleksandr Garagatyi added 2 commits April 24, 2019 14:36
Signed-off-by: Oleksandr Garagatyi <ogaragat@redhat.com>
@garagatyi
Copy link
Author

I have just added discussed changes to the PR

@garagatyi
Copy link
Author

@l0rd please approve to ensure that now our vision are inlined

@l0rd
Copy link
Contributor

l0rd commented Apr 24, 2019

@garagatyi I agree with you about v2/plugins and plugins folders: in this case we want to keep the legacy plugin folder because we want to keep the compatibility with the current ws-master. But that's different from the existing che 7 workspaces configs backward compatibility. That may explain the misunderstanding.

Copy link
Contributor

@l0rd l0rd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@garagatyi garagatyi merged commit 1003e15 into eclipse-che:master Apr 24, 2019
@garagatyi garagatyi deleted the newNotation branch April 24, 2019 15:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants