-
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
Merged
Merged
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Oops, something went wrong.
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back 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.
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 apackage-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 thepackage.json
("node": ">=16"
). We should update that version indeephaven-core/docker/registry/node/gradle.properties
Line 2 in e09bcfd
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