-
Notifications
You must be signed in to change notification settings - Fork 806
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
Merge back into a single lerna monorepo #2892
Conversation
Codecov Report
@@ Coverage Diff @@
## main #2892 +/- ##
==========================================
- Coverage 92.76% 92.48% -0.29%
==========================================
Files 163 151 -12
Lines 5516 4841 -675
Branches 1159 1022 -137
==========================================
- Hits 5117 4477 -640
+ Misses 399 364 -35
|
AFAICT, Lerna can only bump all changed packages to the same version -- this is not what we expect of stable packages and experimental packages release works. Is yarn workspace an available choice? I found that react is using yarn workspace and publishes packages with different versions (scheduler v0.21.0, and react v18.0.0) at one time. |
My current plan is to use lerna independent mode and to create automations to ensure the stable packages are versioned together and the experimental packages are versioned together.
We considered yarn workspaces a long time ago and I can't remember exactly why it was rejected. I think it had to do with supporting old node versions but I could be wrong. NPM now also has workspace support but only for npm 7+. |
Oh, now I know lerna supports independent mode :O. This sounds great and may fit our use case. |
@open-telemetry/javascript-approvers sorry for the delay but this is ready to be reviewed now. Its a lot of files changed but should be relatively easy to review. |
experimental/packages/opentelemetry-api-metrics/src/types/Metric.ts
Outdated
Show resolved
Hide resolved
}, | ||
"devDependencies": { | ||
"@opentelemetry/api": "~1.1.0", | ||
"@opentelemetry/api": "^1.0.0", | ||
"@opentelemetry/context-async-hooks": "1.1.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.
This seems outdated
@@ -54,10 +54,10 @@ | |||
"@opentelemetry/sdk-trace-node": "~1.1.1" |
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.
Is it intended to use ~ here even for components which are now in the same lerna project?
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 we should probably pin them
@@ -42,11 +42,11 @@ | |||
"README.md" | |||
], | |||
"peerDependencies": { | |||
"@opentelemetry/api": ">=1.1.0 <1.2.0", | |||
"@opentelemetry/api": ">=1.0.0 <1.2.0", | |||
"@opentelemetry/api-metrics": "~0.27.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.
Is it intended to have api-metrics as peer dependency here but as dependency in opentelemetry-sdk-metrics-base?
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 confused by the @opentelemetry/api version change. What benefit can we gain on that change?
The specification requires that an SDK support old API versions. This allows users to freely update their SDK without fear that dependencies which use an older API will break. |
experimental/packages/opentelemetry-api-metrics/src/types/Metric.ts
Outdated
Show resolved
Hide resolved
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 fine for lerna but i'm not sure to understand why we changed back to SpanAttributes instead of Attributes ? Is this to support TS user with latest SDK but API <1.1 ?
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.
SGTM
This is attempting to solve several pain points we've had which stem from the fact that our stable and experimental packages are not linked by lerna and aren't released together.
^1.0.0
or>=1.0.0 <1.2.0
in order to facilitate using older APIsKeeping this as a draft for now