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.

Sign up for GitHub

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

Jump to bottom

Set up 'latest' dirs for each existing plugin #143

Merged
merged 3 commits into from
May 24, 2019
Merged

Set up 'latest' dirs for each existing plugin #143

merged 3 commits into from
May 24, 2019

Conversation

amisevsk
Copy link
Contributor

@amisevsk amisevsk commented May 9, 2019

What does this PR do?

Adds script setup_latest_dirs.sh that is used in dockerfiles. This script looks through each plugin's directory for the 'latest' version of each plugin and copies that plugins meta.yaml to a new latest folder.

Rules for deciding which version is newest:

  • If plugin already has a 'latest' folder, nothing is done (assumed to be managed manually)
  • If plugin has a 'next' folder (e.g. che-theia) that meta.yaml is automatically used
  • Otherwise, picks the meta.yaml with newest 'firstPublishedDate'

Only applies to v3 repo path.

Related: redhat-developer/rh-che#1395

@amisevsk amisevsk requested review from l0rd and ibuziuk May 9, 2019 05:22
@amisevsk amisevsk force-pushed the add-latest-dir branch 3 times, most recently from 712da40 to 285974f Compare May 10, 2019 03:45
@l0rd
Copy link
Contributor

l0rd commented May 10, 2019

@amisevsk I am not sure it's a good idea to have that automated. I mean I may want to add a nightly plugin version but don't want it to be latest. And next=latest as well is not a good rule, we are using it right now but we will eventually move to something more stable when 7.0.0 is out.

@amisevsk
Copy link
Contributor Author

@l0rd I agree on the automation aspect, but the alternative is maintaining latest directories manually, which I feel is more error-prone (and will probably be out of date quite quickly). The next = latest hack is mostly because that's what's used for che-theia. Additionally, with this PR having a nightly version that is not latest would only require the maintainer of that plugin to create the latest dir manually.

If we don't want to go in this direction I'm fine with closing this PR, but have reservations about allowing latest versions without some form of automatic checking.

@amisevsk amisevsk mentioned this pull request May 10, 2019
Closed
2 tasks
@benoitf
Copy link
Contributor

benoitf commented May 14, 2019

I also think that next != latest (for che-theia next should be latest stable and not the current master branch for example)

@amisevsk
Copy link
Contributor Author

@benoitf Are we sure on that? We're currently not releasing versions of theia here, so next is the suggested version in general.

@amisevsk
Copy link
Contributor Author

Bit a force-push mess fixing shellcheck issues, but the latest changes are:

  1. Don't automatically generate latest dirs, but leave the script in place so that it can be run manually
  2. Update check script to ignore latest dirs (as those would fail the check)
  3. Add symlinks to latest meta.yamls in each latest folder.

@l0rd PTAL.

@benoitf
Copy link
Contributor

benoitf commented May 14, 2019

@amisevsk the 1.0.0 was used for latest (and it's strange as well as we update continuously 1.0.0 to the che-theia:latest image)

so AFAIK it's more the 1.0.0 version that should be considered as latest as it is using the latest docker image and next which is using the next image

@amisevsk
Copy link
Contributor Author

@benoitf Makes sense; I'll update the PR.

ibuziuk
ibuziuk approved these changes May 16, 2019
View reviewed changes
Copy link
Member

@ibuziuk ibuziuk left a comment

Choose a reason for hiding this comment

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

LGTM, interesting approach with symlinks

l0rd
l0rd approved these changes May 16, 2019
View reviewed changes
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 thanks @amisevsk

@amisevsk
Copy link
Contributor Author

New changes:

  • Added script to ensure every plugin directory contains a latest folder with meta.yaml inside it (necessary to enable support the wsmaster side).
  • Moved all the .sh files into a scripts directory to clean up root folder a bit.

@ibuziuk @l0rd If you have a moment, please reapprove since this changes CI somewhat and could interfere with some of the open PRs we have.

@amisevsk amisevsk requested review from l0rd and ibuziuk and removed request for l0rd May 16, 2019 17:48
@amisevsk
Copy link
Contributor Author

@benoitf Update since I almost forgot: to double check, I should update che-theia/latest to be 1.0.0 and not next? It looks like the latest image on dockerhub is a month old at this point, and we also have PR redhat-developer/rh-che#1414 in rh-che to use next by default.

@l0rd
Copy link
Contributor

l0rd commented May 17, 2019

@amisevsk v3/.../che-theia/1.0.0 should be be replaced (not linked) by v3/.../che-theia/latest. And that's because version 1.0.0 doesn't make sense: the meta.yml points to latest.

That said I would continue to use che-theia/next upstream and on rhche when no explicit editor is specified in a devfile for rc1. We should then switch to latest for rc2 and GA.

@benoitf are you ok with this approach?

@amisevsk
Copy link
Contributor Author

To add a blocker to this being merged: this results in plugins with version latest showing up twice in the user dashboard (since they're indexed twice). This is a TODO, along with the above comment.

@amisevsk
Copy link
Contributor Author

I'm working on the duplicate plugin issue mentioned above and it brings up a question about how we treat theia/latest.

  • If we choose to replace (not link) the meta.yaml, it means that we treat theia differently from the other plugins in the registry
    • We could copy meta.yamls instead of linking for all plugins, although that means information is duplicated with no link
    • We could leave theia as a special case, and choose to support latest with no reference release (similar to docker)
  • Letting latest not be a simple alias to a released version means that the indexing script needs to be significantly reworked -- instead of e.g. skipping latest directories we would have to dedupe the list of plugins in the index.

Another, alternate proposal: create copies of the 'latest' meta.yamls in the latest directory, and rewrite the version in those meta.yamls to be latest. This means managing these latest meta.yamls directly and copying in changes from other meta.yamls manually, which is messy (can no longer easily compare latest meta with newest version), but it would solve the indexing and theia issues directly.

This is a significant downside of our approach to the registry as it is.

@l0rd any thoughts on the matter?

@amisevsk amisevsk force-pushed the add-latest-dir branch 2 times, most recently from 731e831 to 97b96f0 Compare May 22, 2019 16:21
@amisevsk
Copy link
Contributor Author

Okay, version 3 of this PR.

New approach:

  • For each plugin, meta.yaml is a separate file with version latest.
  • The setup_latest_dirs.sh script has been rewritten to copy the newest meta.yaml into the latest directory
    • If no meta.yaml exists, it takes the newest one, sets its version to latest, and copies it
    • If a meta.yaml exists, it compares it to the 'newest' one on a json level
      • If they are different, it prints a warning to the user
      • If they are the same nothing is done
  • eclipse/che-theia/1.0.0 has been renamed to che-theia/latest (this may break compatibility with old workspaces -- is that acceptable?)
  • We validate latest dirs in the same way as others -- they have to contain a plugin with version latest.

@l0rd
Copy link
Contributor

l0rd commented May 23, 2019

eclipse/che-theia/1.0.0 has been renamed to che-theia/latest (this may break compatibility with old workspaces -- is that acceptable?)

I think that almost nobody will be impacted: those that use v3 should use theia next but send an email to che-dev just in case before deploying it on che-plugin-registry.openshift.io

amisevsk added 3 commits May 23, 2019 23:36
@amisevsk
Add script that automatically generates 'latest' dirs for each plugin
0165f26 
Adds script `setup_latest_dirs.sh` that can be used to automatically
latest meta.yamls in each plugin dir. The added meta.yamls are copies of
the meta.yaml with newest 'firstPublishedDate', with version set to be
'latest'.

If a latest directory already exists and a newer version is present
for that plugin, a warning is printed.

Only applies to the v3 registry path.

Also adds script to dockerfiles to ensure that each plugin has a
'latest' version.

Signed-off-by: Angel Misevski <[email protected]>
@amisevsk
Move scripts into separate dir for organization
bfda311 
Move build-related .sh files into scripts dir to clean up root folder of
repo.

Signed-off-by: Angel Misevski <[email protected]>
@amisevsk
Add latest version for existing plugins
d2fe275 
Adds 'latest' version for every existing plugin, which is a copy of the
current latest version with version field overwritten to be 'latest'.

For che-theia, since plugin version 1.0.0 actually points at the
'latest' dockerimage, it is instead renamed to 'latest'. For the
remaining plugins, the newest version listed is unchanged.

Signed-off-by: Angel Misevski <[email protected]>
@amisevsk amisevsk changed the title Add script that automatically generates 'latest' dirs for each plugin Set up 'latest' dirs for each existing plugin May 24, 2019
@amisevsk amisevsk merged commit 675bb4c into eclipse-che:master May 24, 2019
@amisevsk amisevsk mentioned this pull request Jul 17, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@l0rd l0rd l0rd approved these changes

@ibuziuk ibuziuk ibuziuk approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@amisevsk @l0rd @benoitf @ibuziuk