Skip to content
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

Merged
merged 4 commits into from
Feb 12, 2024

Conversation

trentm
Copy link
Member

@trentm trentm commented Feb 9, 2024

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.

…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)
@trentm trentm requested a review from david-luna February 9, 2024 20:15
@trentm trentm self-assigned this Feb 9, 2024
@trentm
Copy link
Member Author

trentm commented Feb 9, 2024

Here is an example run:

% ./scripts/update-otel-deps.js
Updating deps matching "@opentelemetry/*" in 3 workspace dirs

Gathering info on outdated deps in each workspace:
 - packages/opentelemetry-node (1 of 3)
 - packages/mockotlpserver (2 of 3)
 - examples (3 of 3)

Performing updates (2 `npm install ...`s, 1 `npm update ...`):
 $ cd packages/opentelemetry-node && npm install @opentelemetry/[email protected] @opentelemetry/[email protected] @opentelemetry/[email protected] @opentelemetry/[email protected] @opentelemetry/[email protected] @opentelemetry/[email protected]
 $ cd packages/mockotlpserver && npm install @opentelemetry/[email protected] @opentelemetry/[email protected] @opentelemetry/[email protected] @opentelemetry/[email protected]
 $ npm update @opentelemetry/resources @opentelemetry/sdk-metrics

Sanity check that all matching packages are up-to-date:
 $ npm outdated @opentelemetry/exporter-logs-otlp-proto @opentelemetry/exporter-metrics-otlp-proto @opentelemetry/host-metrics @opentelemetry/instrumentation-express @opentelemetry/instrumentation-http @opentelemetry/resources @opentelemetry/sdk-node @opentelemetry/api @opentelemetry/exporter-trace-otlp-grpc @opentelemetry/exporter-trace-otlp-http @opentelemetry/exporter-trace-otlp-proto @opentelemetry/instrumentation-bunyan

Summary of changes (possible commit message):
--
chore(deps): update deps matching "@opentelemetry/*"

    0.34.1 -> 0.35.0 @opentelemetry/host-metrics (range-bump)
    0.34.1 -> 0.35.0 @opentelemetry/instrumentation-express (range-bump)
    0.47.0 -> 0.48.0 @opentelemetry/exporter-logs-otlp-proto (range-bump)
    0.47.0 -> 0.48.0 @opentelemetry/exporter-metrics-otlp-proto (range-bump)
    0.47.0 -> 0.48.0 @opentelemetry/exporter-trace-otlp-grpc (range-bump)
    0.47.0 -> 0.48.0 @opentelemetry/exporter-trace-otlp-http (range-bump)
    0.47.0 -> 0.48.0 @opentelemetry/exporter-trace-otlp-proto (range-bump)
    0.47.0 -> 0.48.0 @opentelemetry/instrumentation-http (range-bump)
    0.47.0 -> 0.48.0 @opentelemetry/sdk-node (range-bump)
    1.20.0 -> 1.21.0 @opentelemetry/resources
--

Copy link
Member

@david-luna david-luna left a 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) {
Copy link
Member

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.

Copy link
Member Author

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.

`${currVer} -> ${latestVer} ${depName} (range-bump)${summaryNote}`
);
} else {
// TODO: Add support looking for a possible update in this case.
Copy link
Member

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Commit 8c3c9f9

@trentm
Copy link
Member Author

trentm commented Feb 12, 2024

node ./scripts/update-deps.js @opentelemetry/* true

and even have a dry-run mode to check before actually doing the changes.

I am not generalizing the options out to command-line options for now, because:

@trentm trentm merged commit 6ea1db8 into main Feb 12, 2024
11 checks passed
@trentm trentm deleted the trentm/update-otel-deps2 branch February 12, 2024 22:29
trentm added a commit that referenced this pull request Feb 12, 2024
Now that we have scripts/update-otel-deps.js for this.

Refs: #68
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants