-
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
Deploy @deephaven/jsapi-types
during release, provide tools for nightly builds
#4907
Conversation
Note that this isn't yet part of CI, either for nightly builds or for tagged releases.
Marked as draft until 0.31 goes out. Two attached example builds for manual review: "beta" for a nightly builds:
deephaven-jsapi-types-1.0.0-dev0.31.0beta20231201.tgz no "preid" for a release:
|
@@ -98,3 +106,9 @@ jobs: | |||
user: __token__ | |||
password: ${{ secrets.PYDEEPHAVEN_PYPI_TOKEN }} | |||
packages_dir: py/client/build/wheel/ | |||
|
|||
- name: Publish @deephaven/jsapi-types to npmjs |
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.
Note that I haven't tested this, all lifted from web-client-ui
def npmVersion = '1.0.0-dev' + project.property('deephavenBaseVersion').toString().trim() | ||
def preId = project.findProperty('npmPreid')// i.e. 'nightly' | ||
if (preId) { | ||
npmVersion += preId + new Date().format("yyyyMMdd") |
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.
We might want to consider hhmmss in here too? Or make it optionally something that could be specified from command line?
This is in lieu of
- ensuring we tag on
main
(which npm insists on for "preid" to work), and - doing a checkout with full history
- copying all of .git into the docker container for npm to find how many commits since the last tag.
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.
Yea probably want to include hhmmss as well, just to avoid any collisions
@@ -1,6 +1,6 @@ | |||
{ | |||
"name": "@deephaven/jsapi-types", | |||
"version": "1.0.0-dev0.31.0", | |||
"version": "0.0.0-dummy", |
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.
explicit dummy version to be clear that developers need not update this, it will automatically be updated in the gradle+docker wiring
.github/workflows/publish-ci.yml
Outdated
with: | ||
node-version-file: 'web/client-api/types/.nvmrc' | ||
registry-url: 'https://registry.npmjs.org' | ||
|
||
# TODO(deephaven-core#2614): Improve gradle/CI assemble and publishing of artifacts | ||
|
||
- name: Publish to Maven Local |
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'd go ahead and update this name in the same way you've updated the latter.
@@ -9,10 +9,11 @@ | |||
"devDependencies": { | |||
"ts-loader": "^6.0.1", | |||
"typescript": "3.9.7", | |||
"webpack": "^5.38.1", | |||
"webpack": "^5.76.0", |
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.
Forgive my dumb questions here, but why 5.76.0 and not one of the patches?
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.
Only because my experience with these tools is limited, and the output does what we need with this 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.
Looking at the package-lock.json, it installed 5.89.0
anyway (specifying the version with a leading ^
prefixed will automatically take any newer minor versions if you're >=1.x, https://semver.npmjs.com/#syntax-examples).
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.
Ok - should I update package.json then, or is it sufficient to say "the combination of the committed package.json and package-lock.json result in a deterministic build"?
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.
No need to update, the combination of package.json/package-lock.json results in a deterministic build.
@@ -10,9 +10,6 @@ | |||
"url": "https://github.com/deephaven/deephaven-core.git", | |||
"directory": "web/client-api/types" | |||
}, | |||
"engines": { | |||
"node": ">=16" | |||
}, | |||
"devDependencies": { | |||
"typedoc": "^0.24.8", | |||
"typescript": "^5.1.6" |
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.
Why is this version different than the one in proto/raw-js-openapi/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.
I'm not going to go so far as to say that I fear change, but I haven't kept up with the changes, and the "inputs" into this typescript process are generated by a tool that are no longer getting updates.
- name: Publish @deephaven/jsapi-types to npmjs | ||
if: ${{ startsWith(github.ref, 'refs/heads/release/v') }} | ||
env: | ||
NODE_AUTH_TOKEN: ${{ secrets.DEEPHAVENBOT_NPM_TOKEN }} |
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.
We'll (I can do this) need to provide deephaven-core repo access to this secret before this action will succeed; I don't know how the npmjs secret management works, but I'm assuming this is a "heavy hammer", but we are okay w/ it @mofojed? The alternative would be a secret that is scoped explicitly for @deephaven/jsapi-types
.
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.
Man I wish npmjs had trusted publishers like PyPi does. Way better workflow.
Yes, the DEEPHAVENBOT_NPM_TOKEN
is not scoped to a particular package, it is a "heavy hammer". You can create granular tokens that are scoped to specific packages, but I think this token scoped to our organization should be fine (organization wide secret) as it would be a pain to have to create new tokens for each package. Or we can have different tokens for the web-client-ui repo, deephaven-plugins repo, and whatever is published by deephaven-core, but then we need to update those tokens anytime we want to publish a new package in that repo (which happens from time to time in both web-client-ui and deephaven-plugins).
https://docs.github.com/en/actions/publishing-packages/publishing-nodejs-packages#publishing-packages-to-the-npm-registry
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.
Done - secret now accessible from deephaven-core.
.github/workflows/publish-ci.yml
Outdated
if: ${{ !startsWith(github.ref, 'refs/heads/release/v') }} | ||
uses: burrunan/gradle-cache-action@v1 | ||
with: | ||
job-id: publish-local | ||
arguments: server-netty-app:build server-jetty-app:build py-server:build py-embedded-server:build py-client:build publishToMavenLocal | ||
arguments: server-netty-app:build server-jetty-app:build py-server:build py-embedded-server:build py-client:build web-client-api:types:typedoc publishToMavenLocal |
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 think assemble.dependsOn(typedoc)
should be added to the web-client-api:types build logic; in which case, this could be updated to web-client-api:types:build
.
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.
Okay, changed - I went a bit further and declared both the npm module tarball and the doc directory as artifacts, which automatically hooks them up as dependent on assemble, just in case we use either in the future in that way.
Builds and passes tests in my web-client-ui branch that consumes these types. Output looks good from the web side |
@@ -9,10 +9,11 @@ | |||
"devDependencies": { | |||
"ts-loader": "^6.0.1", | |||
"typescript": "3.9.7", | |||
"webpack": "^5.38.1", | |||
"webpack": "^5.76.0", |
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.
Looking at the package-lock.json, it installed 5.89.0
anyway (specifying the version with a leading ^
prefixed will automatically take any newer minor versions if you're >=1.x, https://semver.npmjs.com/#syntax-examples).
|
||
runCommand('''set -eux; \\ | ||
cd /project/; \\ | ||
mv dist/types.d.ts dist/index.d.ts; \\ | ||
npm version $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.
Doing this will leave an uncommitted change in package.json - do we need to revert it when we're done?
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.
No - there is no change to the source files, this happens inside docker (and doesn't use volumes/bind mounts).
if: ${{ startsWith(github.ref, 'refs/heads/release/v') }} | ||
env: | ||
NODE_AUTH_TOKEN: ${{ secrets.DEEPHAVENBOT_NPM_TOKEN }} | ||
run: npm publish --tag latest web/client-api/types/build/deephaven-jsapi-types-*.tgz |
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 think you should be referencing a package.json, not a tar file directly... does this actually work?
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 - this is the way to say "here's a finished tarball, go ahead and deploy it". The work of making the tarball is done inside of docker (so that we guarantee it builds the way we expect, that it will do on our own machines), so this npm invocation just has access to the finished tarball and the secret.
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.
Approved for consuming these types. The types built from this branch work w/ my web-client-ui branch https://github.com/mattrunyon/web-client-ui/tree/jsapi-types
Pushing a new merge commit to get CI to re-run, since clicking re-run wasn't doing anything... |
Like other packages, the npmjs package for jsapi types will now get its version assigned as it is built from gradle. Also reads properties from gradle to specify
--preid
to npm (e.g.-PnpmPreid=nightly
), to be given a date stamp suffix.Public job for CI will now deploy tagged versions to npmjs.
Fixes #4888
Fixes #5081