Skip to content

chore: fix build errors#747

Merged
mayurkale22 merged 2 commits intoopen-telemetry:masterfrom
dynatrace-oss-contrib:use-npm
Feb 5, 2020
Merged

chore: fix build errors#747
mayurkale22 merged 2 commits intoopen-telemetry:masterfrom
dynatrace-oss-contrib:use-npm

Conversation

@dyladan
Copy link
Copy Markdown
Member

@dyladan dyladan commented Jan 29, 2020

Fixes an issue that prevented lerna from being used on node 8.

  • Switch from yarn to npm. This will install lerna even on node 8. note: technically we are installing a devDependency that is incompatible with node 8 (lerna), but it is ok because we don't use the feature which uses the broken module.
  • Skip checking the broken link so build will pass.

todo: create a follow up PR that removes the skip of the broken link after it is published to npm #751

@OlivierAlbertini
Copy link
Copy Markdown
Member

I'm reviewing this and on my branch I did exactly the same thing (without changing tdd script) and I get the same issue...

@dyladan
Copy link
Copy Markdown
Member Author

dyladan commented Jan 29, 2020

looking into it

@OlivierAlbertini OlivierAlbertini added the bug Something isn't working label Jan 30, 2020
@dyladan dyladan force-pushed the use-npm branch 2 times, most recently from ccfa941 to 7760b3a Compare February 3, 2020 20:09
@codecov-io
Copy link
Copy Markdown

codecov-io commented Feb 3, 2020

Codecov Report

Merging #747 into master will increase coverage by 0.27%.
The diff coverage is n/a.

@@            Coverage Diff            @@
##           master    #747      +/-   ##
=========================================
+ Coverage   96.22%   96.5%   +0.27%     
=========================================
  Files         208      67     -141     
  Lines        9488    1603    -7885     
  Branches      809     131     -678     
=========================================
- Hits         9130    1547    -7583     
+ Misses        358      56     -302
Impacted Files Coverage Δ
...ges/opentelemetry-propagator-jaeger/src/version.ts 0% <0%> (ø) ⬆️
packages/opentelemetry-api/src/api/metrics.ts 53.33% <0%> (ø) ⬆️
packages/opentelemetry-core/src/common/time.ts 95.38% <0%> (ø) ⬆️
...opentelemetry-core/src/platform/node/timer-util.ts 50% <0%> (ø) ⬆️
...y-api/test/noop-implementations/noop-meter.test.ts 97.22% <0%> (ø) ⬆️
...elemetry-api/src/distributed_context/EntryValue.ts 100% <0%> (ø) ⬆️
packages/opentelemetry-core/src/version.ts 100% <0%> (ø) ⬆️
...ackages/opentelemetry-core/src/platform/node/id.ts 100% <0%> (ø) ⬆️
...core/src/context/propagation/BinaryTraceContext.ts 96.22% <0%> (ø) ⬆️
...try-propagator-jaeger/src/JaegerHttpTraceFormat.ts 95.65% <0%> (ø) ⬆️
... and 197 more

@dyladan dyladan changed the title chore: switch from yarn to npm chore: fix build errors Feb 4, 2020
@dyladan
Copy link
Copy Markdown
Member Author

dyladan commented Feb 4, 2020

@open-telemetry/javascript-approvers I know this has had a lot of updates but I just squashed it all down into two commits. Please take a look and review.

Comment thread packages/opentelemetry-api/package.json
Comment thread .circleci/config.yml
Copy link
Copy Markdown
Member

@mayurkale22 mayurkale22 left a comment

Choose a reason for hiding this comment

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

Nice work, thanks for fixing the build.

@mayurkale22 mayurkale22 added the Merge:LGTM This PR is ready to be merged by a Maintainer (has enough valid approvals, successful build, etc.) label Feb 4, 2020
@mayurkale22 mayurkale22 merged commit 5c9ad85 into open-telemetry:master Feb 5, 2020
@dyladan dyladan mentioned this pull request Feb 5, 2020
@dyladan dyladan deleted the use-npm branch February 5, 2020 17:52
dyladan added a commit to dyladan/opentelemetry-js that referenced this pull request Sep 9, 2022
* chore: switch from yarn to npm

* chore: ignore unpublished package
pichlermarc pushed a commit to dynatrace-oss-contrib/opentelemetry-js that referenced this pull request Dec 15, 2023
* chore: switch from yarn to npm

* chore: ignore unpublished package
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working Merge:LGTM This PR is ready to be merged by a Maintainer (has enough valid approvals, successful build, etc.)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants