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

fix: align dependency versions #3847

Merged
merged 11 commits into from
Jun 12, 2023

Conversation

llc1123
Copy link
Contributor

@llc1123 llc1123 commented Jun 1, 2023

Short description of the changes

Fixed several version mismatches of dependencies.

lerna WARN EHOIST_ROOT_VERSION The repository root depends on [email protected], which differs from the more common semver@^7.3.5.
lerna WARN EHOIST_PKG_VERSION "@opentelemetry/instrumentation-http" package depends on semver@^7.3.5, which differs from the hoisted [email protected].
lerna WARN EHOIST_PKG_VERSION "@opentelemetry/shim-opencensus" package depends on semver@^7.3.5, which differs from the hoisted [email protected].
lerna WARN EHOIST_PKG_VERSION "@opentelemetry/sdk-trace-node" package depends on semver@^7.3.5, which differs from the hoisted [email protected].
lerna WARN EHOIST_PKG_VERSION "@opentelemetry/instrumentation" package depends on semver@^7.3.2, which differs from the hoisted [email protected].
lerna WARN EHOIST_PKG_VERSION "web-opentelemetry-example" package depends on typescript@^4.5.2, which differs from the hoisted [email protected].
lerna WARN EHOIST_PKG_VERSION "@opentelemetry/opentelemetry-browser-detector" package depends on @types/[email protected], which differs from the hoisted @types/[email protected].
lerna WARN EHOIST_PKG_VERSION "backcompat-node14" package depends on @types/[email protected], which differs from the hoisted @types/[email protected].
lerna WARN EHOIST_PKG_VERSION "backcompat-node16" package depends on @types/[email protected], which differs from the hoisted @types/[email protected].
lerna WARN EHOIST_PKG_VERSION "@opentelemetry/exporter-logs-otlp-http" package depends on [email protected], which differs from the hoisted [email protected].
lerna WARN EHOIST_PKG_VERSION "@opentelemetry/sdk-logs" package depends on [email protected], which differs from the hoisted [email protected].
lerna WARN EHOIST_PKG_VERSION "web-opentelemetry-example" package depends on ts-loader@^9.2.6, which differs from the hoisted [email protected].
lerna WARN EHOIST_PKG_VERSION "web-opentelemetry-example" package depends on webpack@^5.65.0, which differs from the hoisted [email protected].
lerna WARN EHOIST_PKG_VERSION "web-opentelemetry-example" package depends on @babel/core@^7.6.0, which differs from the hoisted @babel/[email protected].
lerna WARN EHOIST_PKG_VERSION "web-opentelemetry-example" package depends on babel-loader@^8.0.6, which differs from the hoisted [email protected].
lerna WARN EHOIST_PKG_VERSION "web-opentelemetry-example" package depends on webpack-cli@^4.10.0, which differs from the hoisted [email protected].
lerna WARN EHOIST_PKG_VERSION "web-opentelemetry-example" package depends on webpack-dev-server@^4.5.0, which differs from the hoisted [email protected].
lerna WARN EHOIST_PKG_VERSION "web-opentelemetry-example" package depends on webpack-merge@^5.8.0, which differs from the hoisted [email protected].
lerna WARN EHOIST_PKG_VERSION "@opentelemetry/instrumentation" package depends on require-in-the-middle@^7.1.0, which differs from the hoisted require-in-the-middle@^7.0.0.
lerna WARN EHOIST_PKG_VERSION "@opentelemetry/otlp-grpc-exporter-base" package depends on protobufjs@^7.2.2, which differs from the hoisted protobufjs@^7.1.2.

I will try to refactor web-opentelemetry-example and get rid of webpack@4 (which is incompatible with node 17+) in another PR.

Type of change

  • Bug fix (non-breaking change which fixes an issue)

@llc1123 llc1123 requested a review from a team June 1, 2023 15:15
@codecov
Copy link

codecov bot commented Jun 1, 2023

Codecov Report

Merging #3847 (e7aa2d0) into main (e8540c5) will decrease coverage by 1.16%.
The diff coverage is n/a.

❗ Current head e7aa2d0 differs from pull request most recent head 70a553d. Consider uploading reports for the commit 70a553d to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3847      +/-   ##
==========================================
- Coverage   92.49%   91.34%   -1.16%     
==========================================
  Files         222      120     -102     
  Lines        5610     2587    -3023     
  Branches     1077      543     -534     
==========================================
- Hits         5189     2363    -2826     
+ Misses        421      224     -197     

see 111 files with indirect coverage changes

Copy link
Member

@dyladan dyladan left a comment

Choose a reason for hiding this comment

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

Did you intend to change the ^ dependencies to pinned versions?

experimental/packages/exporter-logs-otlp-http/package.json Outdated Show resolved Hide resolved
@llc1123
Copy link
Contributor Author

llc1123 commented Jun 2, 2023

Yes, just like most packages.

@llc1123 llc1123 force-pushed the fix/version-mismatch branch from 31a6b5a to a2272e4 Compare June 2, 2023 04:41
Copy link
Member

@pichlermarc pichlermarc left a comment

Choose a reason for hiding this comment

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

Looks good, thanks. I'm not entirely sure why some versions were not pinned in the first place 🤔

@Flarna
Copy link
Member

Flarna commented Jun 7, 2023

At least in the past the strategy was to pin only dependencies from the same lerna project as these dependencies are automatically updated on each release.

On the other hand dev-dependencies were pinned to reduce the risk of autobreaking the build and we were not able to use lock files.

After this PR we seem to have some pinned deps (e.g. protobufjs, semver, require-in-the-middle) and some not pinned (e.g. @grpc/grpc-js).

Pinning likely results in duplication of packages for end users because renovate bot tends to fail in this repo and we are slow in updating. in special for bigger packages like @grpc/grpc-js or protobufjs this is usually not nice.

I think we need a reasoning/guideline regarding this.

@llc1123
Copy link
Contributor Author

llc1123 commented Jun 7, 2023

After this PR we seem to have some pinned deps (e.g. protobufjs, semver, require-in-the-middle) and some not pinned (e.g. @grpc/grpc-js).

You are right. I am not sure why some dependencies are pinned and others are not. But at least we should keep the versions of the same package the same. (we have [email protected] somewhere and semver@^7.3.5 elsewhere).

@pichlermarc
Copy link
Member

At least in the past the strategy was to pin only dependencies from the same lerna project as these dependencies are automatically updated on each release.

On the other hand dev-dependencies were pinned to reduce the risk of autobreaking the build and we were not able to use lock files.

Ah, that makes sense.

After this PR we seem to have some pinned deps (e.g. protobufjs, semver, require-in-the-middle) and some not pinned (e.g. @grpc/grpc-js).

Pinning likely results in duplication of packages for end users because renovate bot tends to fail in this repo and we are slow in updating. in special for bigger packages like @grpc/grpc-js or protobufjs this is usually not nice.

I think we need a reasoning/guideline regarding this.

I agree, we should have a guideline for this, I'll look into drafting one up. I think it makes sense to unpin larger packages for the reason you stated.

After this PR we seem to have some pinned deps (e.g. protobufjs, semver, require-in-the-middle) and some not pinned (e.g. @grpc/grpc-js).

You are right. I am not sure why some dependencies are pinned and others are not. But at least we should keep the versions of the same package the same. (we have [email protected] somewhere and semver@^7.3.5 elsewhere).

Agreed. @llc1123 would you mind unpinning protobufjs, require-in-the-middle, @types/shimmer and shimmer again while keeping it at caret versions? Looking into it semver is pinned already in most packages in this repo so I think we can pin this one. This way we'd still align but keep most of what's unpinned still unpinned for now.

@pichlermarc
Copy link
Member

It looks I made a mistake while searching for dependencies onsemver. Looks like it's actually unpinned for dependencies pretty much everywhere but only pinned in devDependencies, which is seems in-line with what Flarna mentioned.

Intuitively I'd say to

@llc1123
Copy link
Contributor Author

llc1123 commented Jun 7, 2023

I've updated the code. I unpinned all dependencies except ones in the lerna project.

@pichlermarc pichlermarc self-requested a review June 7, 2023 15:13
@llc1123 llc1123 force-pushed the fix/version-mismatch branch from fb3b520 to be48adf Compare June 8, 2023 15:11
@llc1123
Copy link
Contributor Author

llc1123 commented Jun 9, 2023

@Flarna What do you think of this version?

@pichlermarc pichlermarc merged commit 3b387d8 into open-telemetry:main Jun 12, 2023
@llc1123 llc1123 deleted the fix/version-mismatch branch June 12, 2023 12:21
Nico385412 pushed a commit to Nico385412/opentelemetry-js that referenced this pull request Jun 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants