-
Notifications
You must be signed in to change notification settings - Fork 80
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
Add notes on releasing jsapi-types #4887
Conversation
Note that this isn't yet part of CI, either for nightly builds or for tagged releases.
024651e
to
7e460d0
Compare
@@ -265,6 +269,7 @@ mention the version explicitly. These files are listed below: | |||
# | |||
gradle.properties | |||
R/rdeephaven/DESCRIPTION | |||
web/client-api/types/package.json |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does package-lock.json need to be updated too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think so, since I never had it in there previously, but I used the npm version
command to perform this particular update and it emitted that. @mofojed can you weigh in?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, otherwise the next npm install
will cause a package-lock.json
diff (when it adds the version info to it). package-lock.json
shouldn't typically be modified by hand though...
Side note - what version of node
/npm
are you using Colin? Doesn't seem to match the engines specified in the package.json
("node": ">=16"
). We should update that version in
deephaven.registry.imageName=node:14 |
node:18
now instead of 14).We should have a .nvmrc file defining the node version of an .npmrc specifying the engine version should be strict and enable provenance as well, so we can sign the builds using GitHub Actions: https://github.blog/2023-04-19-introducing-npm-package-provenance/
I guess then we should just have a gradle action that runs the
npm version
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I used npm 10.2.3 locally to run the npm version
command that wrote that update to the package-lock.json, and must have created the file using the older node/npm from the container, since it produces a different package-lock.json output if i run it again outside that image.
If you're suggesting running npm version from CI, wouldn't that imply either creating a commit, or not updating the version itself?
Also, does provenance buy us anything here, given that it is just the .d.ts file?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bumping the node image breaks the raw-js-openapi build, i'll file an issue and get that worked out separately. I'll back out the version
change to package-lock.json for the moment, and work out the node bump and other CI actions later?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#4888 to follow up on the node image and updating these js projects.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like this fixes #4888 as well now? Looks good to me, but the instructions should be to run the a gradle bump action instead of manually editing this package.json file
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes look good,
@@ -265,6 +269,7 @@ mention the version explicitly. These files are listed below: | |||
# | |||
gradle.properties | |||
R/rdeephaven/DESCRIPTION | |||
web/client-api/types/package.json |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like this fixes #4888 as well now? Looks good to me, but the instructions should be to run the a gradle bump action instead of manually editing this package.json file
b1bc4b3
to
7786b50
Compare
Removed last commit, will make a separate PR for that after the release. |
Note that this isn't yet part of CI, either for nightly builds or for tagged releases.