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

fix: consume generator tool from upstream as an npm package #564

Merged
merged 5 commits into from
Aug 12, 2021

Conversation

vinokurig
Copy link
Contributor

Signed-off-by: Igor Vinokur ivinokur@redhat.com

What does this PR do?

Consume generator tool from upstream as an npm package

What issues does this PR fix or reference?

https://issues.redhat.com/browse/CRW-1928

Release Notes

Docs PR (if applicable)

Signed-off-by: Igor Vinokur <ivinokur@redhat.com>
Copy link
Contributor

@ericwill ericwill left a comment

Choose a reason for hiding this comment

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

This is for 2.11 right? Shouldn't this PR be against the crw-2.11-rhel8 branch then?

dependencies/che-plugin-registry/build.sh Outdated Show resolved Hide resolved
Copy link
Member

@nickboldt nickboldt left a comment

Choose a reason for hiding this comment

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

This PR is for crw-2-rhel-8 = 2.12, so as it stands I'm OK with it... though the JIRA says fixversion = 2.11, so we'll need a second PR for the 2.11 branch.

I also don't love that there are only "-dev-" versions in https://www.npmjs.com/package/@eclipse-che/plugin-registry-generator for the 2.11 release, I'd prefer to see a stable release at npmjs. Can we do some 7.yy.z releases in addition to the nightly devs?

We'll need a new Prod Sec exception to be noted about this change, however, as it's a new npm dependency that we build (but which is hosted by 3rd party). https://docs.google.com/document/d/1nw_O1QeG5bg1jfpoSkmABNiCjLpxxc9bSQXXLo8jgJw/edit#heading=h.ttxuwnu2u1jf

How will we ensure we're using the latest correct version of the package in 2.x and 2.11 branches?

Can we add this version dependency into https://github.com/redhat-developer/codeready-workspaces/blob/crw-2-rhel-8/dependencies/VERSION.json and add some build process that pulls the version from there to make sure we build with the correct version?

@nickboldt nickboldt self-requested a review August 10, 2021 20:06
Copy link
Member

@nickboldt nickboldt left a comment

Choose a reason for hiding this comment

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

Can you collect the version of the generator from a centalized place, eg.,

 ECPRG_VER=$(curl -sSLo- https://raw.githubusercontent.com/redhat-developer/codeready-workspaces/crw-2-rhel-8/dependencies/VERSION.json \
| yq -r '.Other["@eclipse-che/plugin-registry-generator"]["2.12"]'); echo $ECPRG_VER

@vinokurig
Copy link
Contributor Author

@nickboldt

Can you collect the version of the generator from a centalized place, eg.,

ECPRG_VER=$(curl -sSLo- https://raw.githubusercontent.com/redhat-developer/codeready-workspaces/crw-2-rhel->8/dependencies/VERSION.json \
| yq -r '.Other["@eclipse-che/plugin-registry-generator"]["2.12"]'); echo $ECPRG_VER

Done.

@svor
Copy link
Contributor

svor commented Aug 11, 2021

ECPRG_VER=$(curl -sSLo- https://raw.githubusercontent.com/redhat-developer/codeready-workspaces/crw-2-rhel-8/dependencies/VERSION.json
| yq -r '.Other["@eclipse-che/plugin-registry-generator"]["2.12"]'); echo $ECPRG_VER

@nickboldt I don't think it is good to have hardcoded version in the script. But if we do that, shouldn't we use 2.x for crw-2-rhel-8 branch?

And it would be better to move that value into the varible in the script.

@nickboldt
Copy link
Member

hardcoded version in the script.

Agreed, better to pull the version from https://github.com/redhat-developer/codeready-workspaces/blob/crw-2-rhel-8/dependencies/VERSION.json#L2

That way it'll automatically be updated to 2.13 when we move into the 2.12 branch and update 2.x to a newer future version

@benoitf
Copy link
Contributor

benoitf commented Aug 11, 2021

@nickboldt

I also don't love that there are only "-dev-" versions in https://www.npmjs.com/package/@eclipse-che/plugin-registry-generator for the 2.11 release, I'd prefer to see a stable release at npmjs. Can we do some 7.yy.z releases in addition to the nightly devs?

It was the goal. It's just that there was a bug that has been fixed (release script was failing to publish to npmjs) so 7.34.2 should be there when you'll cut it

use version from json .Version to compute .Other["@eclipse-che/plugin-registry-generator"][$REGISTRY_VERSION]; use local file instead of curling from another project/branch
@nickboldt
Copy link
Member

ready to merge in both 2.11 and 2.x branches (change is now the same in both places and can simply compute its version and associated generator version, as long as the VERSION.json file in each branch is kept up to date.

@nickboldt nickboldt self-requested a review August 11, 2021 21:44
@nickboldt
Copy link
Member

related: eclipse-che/che-plugin-registry#1027

@svor svor merged commit 12819f1 into crw-2-rhel-8 Aug 12, 2021
@svor svor deleted the crw-1928 branch August 12, 2021 15:05
nickboldt added a commit to redhat-developer/devspaces-images that referenced this pull request Aug 12, 2021
Signed-off-by: nickboldt <nboldt@redhat.com>
nickboldt added a commit to redhat-developer/devspaces-images that referenced this pull request Aug 12, 2021
Signed-off-by: nickboldt <nboldt@redhat.com>
@nickboldt
Copy link
Member

Had to do a few more things for this to work, including moving to 0.0.2 version of che-openshift-auth plugin (as the 0.0.1 no longer exists). eclipse-che/che-plugin-registry#1028

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.

5 participants