-
Notifications
You must be signed in to change notification settings - Fork 70
chore(ci): add individual publishing of packages MONGOSH-1871 #2289
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
Conversation
879eb0f
to
40d6e71
Compare
f48cc5c
to
a2f246b
Compare
8c9c214
to
41bc10a
Compare
@addaleax Didn't get to spend too much time on this before my holiday with some VSCode and coverage test back and forths. Would be happy to jump back into it in a week but if it'd be better to finish this earlier, feel free to pick it up 🙂 I have some WIP files that I may polish up and push at a later time also though not sure how useful they are; will see. |
41bc10a
to
ad630dc
Compare
.evergreen/evergreen.yml.in
Outdated
@@ -917,6 +917,9 @@ functions: | |||
{ | |||
export NODE_JS_VERSION=${node_js_version} | |||
source .evergreen/setup-env.sh | |||
npm run evergreen-release bump | |||
git add . | |||
git commit --no-allow-empty -m "chore(release): bump to prepare for mongosh release" |
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.
The idea here is that this will bump both "independent" and mongosh packages before the release draft and push it to main, so our branch is consistent with what we're releasing.
It might also make sense to only persist this after the release is published.
I am assuming this should work? Though not sure if the evergreen git setup has ability to push to main like this?
export const MONGOSH_RELEASE_PACKAGES = ['mongosh', '@mongosh/cli-repl']; | ||
|
||
/** Packages which always get ignored when doing a release or bump */ | ||
export const IGNORE_BUMP_PACKAGES = [ |
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.
ignoring these since these are private, unless it'd make sense to publish anyways?
packages/arg-parser/package.json
Outdated
@@ -1,6 +1,6 @@ | |||
{ | |||
"name": "@mongosh/arg-parser", | |||
"version": "0.0.0-dev.0", | |||
"version": "2.3.7", |
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.
these were all set to 2.3.7 manually to get the current repository consistent with the latest release
3c557da
to
68e3e62
Compare
@@ -1,6 +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.
I don't know where we need to to leave this comment, so I'll leave it here – in shell-api.ts
, we fetch the current mongosh version via require('../package.json').version
. We should replace that with a new method on EvaluationListener
that we then call from there, so that the mongosh CLI can provide the correct version to the shell-api package (and probably also add an e2e test that mongosh --eval 'version()'
and mongosh --version
print the same output).
packages/e2e-tests/test/e2e.spec.ts
Outdated
shell.assertContainsOutput(expected); | ||
shell.assertNoErrors(); | ||
shell.assertContainsOutput(expectedPackageVersion); | ||
shell.assertContainsOutput(expectedVersionFromRepl); |
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.
Not a huge deal, but I think it would be a tad cleaner to use really compare version()
against the output of mongosh --version
instead of adding more explicit source dependencies on the cli-repl package contents (and even internals) here.
packages/shell-api/src/shell-api.ts
Outdated
const version = require('../package.json').version; | ||
const version = this._instanceState.evaluationListener.version; | ||
if (!version) { | ||
throw new MongoshInternalError('mongosh version not known'); |
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.
Scripts don't expect version()
to throw, I don't think we can do this here
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.
Hm fair, I expect this to hypothetically never happen, just handled it this way to respect its possibly undefined type, would it be better to ignore it (I guess then returning undefined) or some placeholder i.e. 0.0.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.
Hm yeah. If we do want to rely on this property always present, let's make it an actual required property and ensure that a value gets provided in Compass and the java-shell implementation, otherwise a placeholder like 0.0.0
is probably a good idea
But thinking about it more, especially in conjunction with the concern about the other non-CLI products, maybe we should just find an easy way to use the version of the cli-repl package here, that's what most people will be thinking of as the "mongosh version" anyway (even though then multiple shell-api versions can refer to the same CLI 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 am realizing that expecting a preset version
on EvaluationListener
does mean browser-repl
, etc. need to define it also; or should it just be a required field in MongoshNodeRepl
?
Would it be a realistic thing to depend on a programmatically set environment variable for MONGOSH_VERSION
and/or a global variable for browser-based contexts? Not sure how it'd work in a Java / IntelliJ context.
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 mean
browser-repl
, etc. need to define it also
Yeah, that's kind of what I'm getting at, right now version()
returns a meaningful value in e.g. Compass, and we probably want to keep that behavior around
Would it be a realistic thing to depend on a programmatically set environment variable for MONGOSH_VERSION and/or a global variable for browser-based contexts? Not sure how it'd work in a Java / IntelliJ context.
Well, the EvaluationListener
interface is our interface for getting this type of information into the executable, so we should be using that.
But yeah, ultimately I think people would want to see a version that correlates with the mongosh CLI version that corresponds to the current version of the shell-api package. This could also be something like the compile
step in shell-api
writing down the cli-repl
package version somewhere inside of the package
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.
Should it be in the compile
step or as part of the mongosh release process since that's when that value would change anyways?
Along with the package.json
-s bump we could update a constant inside shell-api
and bump it along with that to main. Then there won't be need for any changes to EvaluationListener
, right?
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.
That would also work, yes. I figured compile
would be easier since it requires maintaining less state, but it does have the downside of adding a new generated file to the repository, so no strong feelings about which solution we go with (but we should do either of them 🙂)
@@ -56,6 +59,7 @@ export class WorkerThreadEvaluationListener { | |||
(Promise.resolve() as Promise<never>) | |||
); | |||
}, | |||
version: workerRuntime.evaluationListener?.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.
How do we provide this value in non-CLI products (vscode/Compass/IntelliJ)? I do think that's something we're going to have to figure out, and unfortunately I don't have an obvious answer for it myself
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.
if I understand correctly, making it a generated constant inside shell-api
essentially handles that right?
a9d2422
to
ed9ffa1
Compare
ed9ffa1
to
e2ceaa5
Compare
de993f9
to
9c4c358
Compare
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 reasonable, I have mostly very minor comments. One thing I was wondering is - this looks like it'll tag all the auxiliary packages released, but won't create a github release for those. Do we want to also create releases, or do we think it'll get too noisy if we did it for every package?
- name: Create Github App Token | ||
uses: mongodb-js/devtools-shared/actions/setup-bot-token@main | ||
id: app-token | ||
with: | ||
app-id: ${{ vars.DEVTOOLS_BOT_APP_ID }} | ||
private-key: ${{ secrets.DEVTOOLS_BOT_PRIVATE_KEY }} |
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 do we need this? Is it because publish-auxiliary pushes some tags that we want to configure the git user for?
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 wasn't sure if the git workflow can just push anything at all. To be honest the relationship between the bot token and just an action is a bit unclear to me, might be nice to have a quick explainer on that sometime
Co-authored-by: Nikola Irinchev <[email protected]>
@nirinchev I think it’d be better to keep releases exclusive to mongosh to keep a clean timeline of releases for users to look into; which is also how we do it in Compass. The tags + PRs should be nice enough for our internal reference I’d think. Though open to adding some other form cc: @addaleax |
This adds the ability to individually publish packages in mongosh. Also, notably, all packages's version inside package.json will now match the latest released version and update accordingly upon release.
Structure
Package Bumping
With this change, there are 2 types of package bumping depending on what package it is:
mongosh
and@mongosh/cli-repl
are bumped only at mongosh release and have their versions synced.This gets triggered only during a mongosh release.
All other packages are bumped independently of mongosh using
bump-packages
from monorepo-tools. Each package can have divergent versions as a result.This can be triggered anytime using GitHub actions which will create PR for bump package versions, merging of which will cause CI to publish the packages.
Individual Package Release
You can release an individual package by running a bump packages GitHub Action and merging the created PR.
mongosh Release
A mongosh release will cause both an npm package bump and a mongosh release bump before starting the release.