-
Notifications
You must be signed in to change notification settings - Fork 3
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: add scripts/update-otel-deps.js to carefully handle updating @opentelemetry/* deps #68
Conversation
…opentelemetry/* deps The motivation here was that dependabot doesn't handle this group update for a repo using npm workspaces (see #58 and experiences with opentelemetry-js-contrib, open-telemetry/opentelemetry-js-contrib#1917)
Here is an example run:
|
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 ad proficient as you on npm
mechanic but I see you did this PR thoroughly. I left some comments since I saw potential to use it more as a CLI tool than a specific script in our repo.
I wouldn't mind to type from time to time
node ./scripts/update-deps.js @opentelemetry/* true
and even have a dry-run mode to check before actually doing the changes.
console.log(`Updating deps${matchStr} in ${wsDirs.length} workspace dirs`); | ||
|
||
const depPatternFilter = (name) => { | ||
if (!patterns) { |
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 patterns
is not defined this script will try to update all dependencies right? I think maybe is better the script requires at least one pattern so changes made by it are focused in scope. This means also to not allow the wildcard pattern to match all.
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.
Added a guard in commit 8c3c9f9. I'm not guarding against passing in *
for now.
scripts/update-otel-deps.js
Outdated
`${currVer} -> ${latestVer} ${depName} (range-bump)${summaryNote}` | ||
); | ||
} else { | ||
// TODO: Add support looking for a possible update in this case. |
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.
nit: the condition allowRangeBumpFor0x === false && semver.lt(currVer, '1.0.0')
falls in this else
block. We may want to tell the user that he/she can enable the flag to get an update.
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.
Commit 8c3c9f9
I am not generalizing the options out to command-line options for now, because:
|
Now that we have scripts/update-otel-deps.js for this. Refs: #68
The motivation here was that dependabot doesn't handle this group update for
a repo using npm workspaces (see #58
and experiences with opentelemetry-js-contrib, open-telemetry/opentelemetry-js-contrib#1917)
David, I'm not sure how closely you care to review this. I'll do a separate PR that actually uses this to update our OTel deps. I've been testing this some with opentelemetry-js-contrib as well for open-telemetry/opentelemetry-js-contrib#1917 -- which is where some of the defensive coding comes from.