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 prepublishOnly to ensure a full build #2844

Merged
merged 5 commits into from
Mar 22, 2022

Conversation

legendecas
Copy link
Member

@legendecas legendecas commented Mar 18, 2022

Which problem is this PR solving?

Fixes: #2843

Add npm script prepublishOnly to run a full build before publishing. prepare may be a good alternative but it also runs on npm install -- it may slow down the install step.

These build variants have already been tested in our GitHub Action suites.

Refs: https://docs.npmjs.com/cli/v8/using-npm/scripts#life-cycle-operation-order

Short description of the changes

  • Also add the missing esnext config for propagator-jaeger and the template package.
  • Update dependency @opentelemetry/api of @opentelemetry/sdk-node to ~1.1.0.

Type of change

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

Checklist:

  • Followed the style guidelines of this project

@legendecas legendecas requested a review from a team as a code owner March 18, 2022 15:35
@codecov
Copy link

codecov bot commented Mar 18, 2022

Codecov Report

Merging #2844 (45ac10f) into main (8182dab) will increase coverage by 1.32%.
The diff coverage is n/a.

❗ Current head 45ac10f differs from pull request most recent head 9745804. Consider uploading reports for the commit 9745804 to get more accurate results

@@            Coverage Diff             @@
##             main    #2844      +/-   ##
==========================================
+ Coverage   93.52%   94.85%   +1.32%     
==========================================
  Files         163       16     -147     
  Lines        5572      602    -4970     
  Branches     1180      107    -1073     
==========================================
- Hits         5211      571    -4640     
+ Misses        361       31     -330     
Impacted Files Coverage Δ
...emetry-api-metrics/src/platform/node/globalThis.ts 0.00% <0.00%> (-100.00%) ⬇️
packages/opentelemetry-core/src/utils/merge.ts
...strumentation/src/platform/node/instrumentation.ts
...ics-base/src/exemplar/NeverSampleExemplarFilter.ts
...emetry-sdk-metrics-base/src/state/MetricStorage.ts
...ges/opentelemetry-resources/src/detectors/index.ts
...kages/opentelemetry-core/src/trace/rpc-metadata.ts
...metry-sdk-metrics-base/src/InstrumentDescriptor.ts
packages/opentelemetry-core/src/utils/callback.ts
...ackages/opentelemetry-shim-opentracing/src/shim.ts
... and 138 more

@legendecas
Copy link
Member Author

The test is failing. It seems like the gcp-detector is not behaving in an expected way...

@legendecas
Copy link
Member Author

legendecas commented Mar 18, 2022

Well, it seems the problem is related to @opentelemetry/api version mismatch: @opentelemetry/sdk-node uses ~1.0.3, whilst @opentelemetry/resource-detector-gcp uses v1.1.0 (^1.0.2)

@vmarchaud
Copy link
Member

I think we might consider moving the sdk node to the contrib since all detectors live there

@legendecas
Copy link
Member Author

legendecas commented Mar 19, 2022

I believe we'll need a version bump on dependencies of experimental packages to get the CI working.

Edit: well, no need to do so. We have only @opentelemetry/sdk-node that uses ~1.0.3 notation on @opentelemetry/api.

Copy link
Member

@Flarna Flarna left a comment

Choose a reason for hiding this comment

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

What is the main difference between compile and publishOnly script?

@legendecas
Copy link
Member Author

@Flarna What is the main difference between compile and publishOnly script?

The publishOnly is an npm hook that is executed before the npm publish command (See https://docs.npmjs.com/cli/v8/using-npm/scripts#life-cycle-operation-order for more details), while compile is not an npm hooks.

I'd wait for @dyladan 's opinion on this since he is the one who performs the releases.

@legendecas legendecas requested a review from dyladan March 20, 2022 03:18
@Flarna
Copy link
Member

Flarna commented Mar 20, 2022

Isn't it prepublishOnly?

@legendecas
Copy link
Member Author

@Flarna thank you for pointing that out! That was a silly mistake I've made :(

@legendecas legendecas changed the title chore: add publishOnly to ensure a full build chore: add prepublishOnly to ensure a full build Mar 20, 2022
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.

Thanks for adding this. We'll have to cut a 1.1.1 release to fix the release ASAP

@dyladan
Copy link
Member

dyladan commented Mar 22, 2022

I'd wait for @dyladan 's opinion on this since he is the one who performs the releases.

I'm working locally on a branch to automate the releases as I don't want to do it forever

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.

NPM packages are missing "esm" and "esnext" builds
4 participants