-
Notifications
You must be signed in to change notification settings - Fork 616
chore: upgrade lerna #2836
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
chore: upgrade lerna #2836
Conversation
pichlermarc
left a comment
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 good % a few nits, thanks for working on this 🙂
I think RELEASING.md can be removed as we likely won't release manually anymore now that we also add provenance statements to our published packages.
| popd | ||
| done | ||
| ```sh | ||
| npx lerna publish from-package --no-push --no-private --no-git-tag-version --no-verify-access --yes |
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 the whole RELEASING.md can be removed, or we can refer to the release-please workflow that basically handles all the releases now. 🙂
(In the 2.5 years I've been a maintainer, I've never needed to do a manual release in the contrib repo 🙂)
| "watch": "tsc --build --watch tsconfig.json tsconfig.esm.json tsconfig.esnext.json", | ||
| "precompile": "lerna run version --scope @opentelemetry/propagator-aws-xray --include-dependencies", | ||
| "prewatch": "npm run precompile" | ||
| "watch": "tsc --build --watch tsconfig.json tsconfig.esm.json tsconfig.esnext.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.
Q: I am ignorant here. Does removing this precompile mean that I see the npm run compile at the top-level before working on this package? (Maybe that is already a requirement.)
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.
(Maybe that is already a requirement)
That's what I interpreted from the contributing guide
CONTRIBUTING.md
Outdated
| - `npm run lint:fix` lint any changes and fix if needed. | ||
|
|
||
| Each of these commands can also be run in individual packages, as long as the initial install and compile are done first in the root directory. | ||
|
|
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 it woudl be helpful to have a separate section header for this new focus thing.
| popd | ||
| done | ||
| ```sh | ||
| npx lerna publish from-package --no-push --no-private --no-git-tag-version --no-verify-access --yes |
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.
From the top of this file:
For posterity, or in the event of any failures with release-please, the process for performing a manual release is below.
Given this gets out of date, it would possibly be better to drop it. I'd defer to @pichlermarc for an opinion on that as he does most of the releasing.
Update: Marc already gave an opinion on this above (#2836 (comment)).
CONTRIBUTING.md
Outdated
| > NX Successfully ran target compile for project @opentelemetry/resource-detector-aws and 3 tasks it depends on (7s) | ||
| ``` | ||
|
|
||
| Once the command is done you can `cd` into the package and start using the usual commands. |
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.
(non-blocking) I'm not opposed to this command, but it would be kind of nice to not have to type out both (a) the package name and (b) the directory for that package.
Possible alternative:
cd detectors/node/opentelemetry-resource-detector-aws # or whatever package to focus on
npm run compile:include-deps
Or perhaps others would find a command at the top-level more convenient? My personal typical development process is that I cd into the package dir I am working on and work from there. So an npm run ... script in that package dir would be convenient for me.
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.
Since I was already removing scripts from each package.json I didn't want to add anything else. But I think its a fair point. At the end you have to cd into that directory to run the scripts there so it would be more convenient.
Checklist:
- remove
focusscript - add
setup:devscript in each package - update
contributing.md: remove focus section - update
contributing.md: rewrite compile at the top requirement
Co-authored-by: Marc Pichler <[email protected]>
Co-authored-by: Marc Pichler <[email protected]>
|
There is a test failing in |
Co-authored-by: Trent Mick <[email protected]>
Which problem is this PR solving?
This is a follow up PR of #2493. It updates
lernato version v8.2.2, which indirectly updatesnx. Dev scripts are usingnxdirectly but publish workflows and tasks keep usinglernaChanges
lernadependencyscripts/peer-api-check.mjsto work at the root level and the dependant workflowsscripts/check-release-please.mjs, simplificationsetup:devnpm script in each package for people who does not want to compile all project to start developingSupersedes: #2526