ci: add npm cache in actions/setup-node #4271

lyzlisa · 2023-11-08T21:46:23Z

Which problem is this PR solving?

Please include a summary of the change and which issue is fixed. Please also include relevant motivation and context. List any dependencies that are required for this change.

Fixes #4243

Short description of the changes

Enable cache in actions/setup-node

Type of change

Please delete options that are not relevant.

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
This change requires a documentation update

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

Test A

Checklist:

Followed the style guidelines of this project
Unit tests have been added
Documentation has been updated

linux-foundation-easycla · 2023-11-08T21:46:28Z

The committers listed above are authorized under a signed CLA.

✅ login: lyzlisa / name: lyzlisa (35226ff, 6d4cd9e, bd7d3cb, 5da12f8)
✅ login: pichlermarc / name: Marc Pichler (be4456b)
✅ login: trentm / name: Trent Mick (49b33f4, a085295, 1fb2d00)

codecov · 2023-11-10T08:51:32Z

Codecov Report

Merging #4271 (1fb2d00) into main (6dd075c) will not change coverage.
The diff coverage is n/a.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #4271   +/-   ##
=======================================
  Coverage   92.20%   92.20%           
=======================================
  Files         332      332           
  Lines        9439     9439           
  Branches     2000     2000           
=======================================
  Hits         8703     8703           
  Misses        736      736

.github/workflows/lint.yml

lyzlisa · 2023-11-10T14:19:13Z

.github/workflows/benchmark.yml

@@ -17,10 +17,13 @@ jobs:
      NPM_CONFIG_UNSAFE_PERM: true
    steps:
      - name: Checkout
-        uses: actions/checkout@v3
+        uses: actions/checkout@v4


May be out of scope for this PR, but all other workflows touched in this PR were already using v4. Let me know if it makes more sense to leave it out of this PR.

I think it makes sense to keep it in this PR👍
Most likely it was an oversight.

pichlermarc

Looks great, thanks 🙂

pichlermarc · 2023-11-13T12:48:45Z

.github/workflows/benchmark.yml

@@ -17,10 +17,13 @@ jobs:
      NPM_CONFIG_UNSAFE_PERM: true
    steps:
      - name: Checkout
-        uses: actions/checkout@v3
+        uses: actions/checkout@v4


I think it makes sense to keep it in this PR👍
Most likely it was an oversight.

legendecas · 2023-11-29T09:21:29Z

The CI failure was caused by https://github.com/open-telemetry/opentelemetry-js/blob/8067d9721021cb25f94534a27cd2e6298ef7f6a2/.npmrc#L1C10-L1C10, which would cause npm bundled with node v14 and lower to abort.

I think the NPM_AUTH_TOKEN (example) can be an alternative to it.

pichlermarc · 2023-11-29T15:13:52Z

The CI failure was caused by https://github.com/open-telemetry/opentelemetry-js/blob/8067d9721021cb25f94534a27cd2e6298ef7f6a2/.npmrc#L1C10-L1C10, which would cause npm bundled with node v14 and lower to abort.

Ah that's my fault. I added this - I'll remove it again. It's not yet used anyway. (PR to remove it: #4329)

trentm · 2023-11-29T16:25:16Z

.github/workflows/lint.yml

+          cache-dependency-path: |
+            package-lock.json


https://github.com/actions/setup-node#caching-global-packages-data suggests that package-lock.json will be used as a default:

The action defaults to search for the dependency file (package-lock.json, npm-shrinkwrap.json or yarn.lock) in the repository root, and uses its hash as a part of the cache key. Use cache-dependency-path for cases when multiple dependency files are used, or they are located in different subdirectories.

However, I think it is fine either way.

trentm · 2023-11-29T16:27:05Z

.github/workflows/lint.yml

      - uses: actions/setup-node@v4
        with:
+          cache: 'npm'


I'm curious if we are able to notice any speed up in npm ci steps in this or other workflows as a result of caching. With the variable GitHub Action runners it may be hard to get reliable numbers, though.

From what I can tell from a quick small number of checks at https://github.com/open-telemetry/opentelemetry-js/actions/workflows/unit-test.yml for node-test (v18), the run time reduced from 2m35s to about 2m with this PR.

trentm · 2023-12-01T18:32:13Z

Thanks @lyzlisa!

* Add Trent to approvers (#4311) * chore(renovate): require dashboard approval for lerna updates (#4276) * chore(ci): install semver globally to speed up "peer-api" workflow (#4270) Closes: #4242 * fix(ci): remove token setup via environment variable from .npmrc (#4329) * fix(instrumentation-http): resume responses when there is no response listener Fixes a memory leak where unhandled response bodies pile up in node 20 * feat: add script to update changelogs on release preparation (#4315) * feat: add script to update changelogs on releases * fix: address comments * Apply suggestions from code review Co-authored-by: Trent Mick <trentm@gmail.com> * fix: apply suggestions from code review * fix: use packageJson.version instead of version --------- Co-authored-by: Trent Mick <trentm@gmail.com> * Fix event name * test: make rawRequest HTTP-compliant * Add node 20 to test matrix * Enable old hash functions on 20 * Fix esm handling for iitm node 20 * Use err.code to make test more reliable * Changelog * nit: single import * Remove unused files * Add v20 to supported runtimes * ci: add npm cache in actions/setup-node (#4271) * feat(sdk-logs): add droppedAttributesCount field to ReadableLogRecord (#4289) * feat(sdk-logs): add droppedAttributesCount field to ReadableLogRecord * chore: check droppedAttributesCount value in test case * feat(otlp-transformer): make toLogRecord() use ReadableLogRecord.droppedAttributesCount --------- Co-authored-by: Marc Pichler <marc.pichler@dynatrace.com> * fix(api-logs): allow passing in TimeInput for LogRecord (#4345) * fix: allow passing in TimeInput for LogRecord * chore: update changelog * fix: programmatic url and headers take precedence in metric exporters… (#4334) * fix: programmatic url and headers take precedence in metric exporters (#2370) * chore: adjust grpc exporter metrics test * chore(changelog): update changelog * fix(instrumentation-http): do not mutate given headers object for outgoing http requests (#4346) Fixes: open-telemetry/opentelemetry-js-contrib#1609 * chore(deps): update actions/stale action to v9 (#4353) * fix(deps): update dependency import-in-the-middle to v1.6.0 (#4357) * chore(deps): update all patch versions (#4306) * chore(ci): use node 20 in lint workflow (#4359) * chore(deps): update dependency linkinator to v6 (#4237) * fix(otlp-exporter-base): decrease default concurrency limit to 30 (#4211) * fix(otlp-exporter-base): decrease concurrency limit to 30 * fix(changelog): add changelog entry * chore(deps): use actions/checkout >4 instead of 4.0.0 exactly (#4361) --------- Co-authored-by: Marc Pichler <marc.pichler@dynatrace.com> Co-authored-by: strivly <133362191+strivly@users.noreply.github.com> Co-authored-by: Trent Mick <trentm@gmail.com> Co-authored-by: lyzlisa <34400837+lyzlisa@users.noreply.github.com> Co-authored-by: Hyun Oh <hyunnoh01@gmail.com> Co-authored-by: Siim Kallas <siimkallas@gmail.com> Co-authored-by: Vladimir Adamić <441333+Vunovati@users.noreply.github.com> Co-authored-by: Mend Renovate <bot@renovateapp.com>

lyzlisa requested a review from a team as a code owner November 8, 2023 21:46

lyzlisa changed the title ~~ci(opentelemetry-core): add npm cache in actions/setup-node@…~~ ci(opentelemetry-core): add npm cache in actions/setup-node@v3 Nov 8, 2023

lyzlisa force-pushed the gh-action-add-npm-ci branch 2 times, most recently from 8bfa115 to 2cd21d6 Compare November 9, 2023 18:13

pichlermarc reviewed Nov 10, 2023

View reviewed changes

.github/workflows/lint.yml Outdated Show resolved Hide resolved

pichlermarc added the Skip Changelog label Nov 10, 2023

lyzlisa commented Nov 10, 2023

View reviewed changes

lyzlisa force-pushed the gh-action-add-npm-ci branch 2 times, most recently from 762e549 to ed2d410 Compare November 12, 2023 01:31

lyzlisa added 4 commits November 23, 2023 13:16

ci(opentelemetry-core): add npm cache in actions/setup-node@v3

35226ff

Move checkout step before setup-node

6d4cd9e

Add name to checkout step in lint.yml for consistency across all actions

bd7d3cb

Bump actions/checkout to v4 in benchmark.yml

5da12f8

lyzlisa force-pushed the gh-action-add-npm-ci branch from ed2d410 to 5da12f8 Compare November 23, 2023 18:17

pichlermarc approved these changes Nov 27, 2023

View reviewed changes

Merge branch 'main' into gh-action-add-npm-ci

be4456b

pichlermarc mentioned this pull request Nov 29, 2023

fix(ci): remove token setup via environment variable from .npmrc #4329

Merged

trentm reviewed Nov 29, 2023

View reviewed changes

Merge branch 'main' into gh-action-add-npm-ci

49b33f4

trentm approved these changes Nov 30, 2023

View reviewed changes

trentm added 2 commits November 30, 2023 10:33

Merge branch 'main' into gh-action-add-npm-ci

a085295

Merge branch 'main' into gh-action-add-npm-ci

1fb2d00

trentm merged commit 9e94536 into open-telemetry:main Dec 1, 2023
20 checks passed

trentm changed the title ~~ci(opentelemetry-core): add npm cache in actions/setup-node@v3~~ ci: add npm cache in actions/setup-node Dec 1, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ci: add npm cache in actions/setup-node #4271

ci: add npm cache in actions/setup-node #4271

lyzlisa commented Nov 8, 2023 •

edited

Loading

linux-foundation-easycla bot commented Nov 8, 2023 •

edited

Loading

codecov bot commented Nov 10, 2023 •

edited

Loading

lyzlisa Nov 10, 2023

pichlermarc Nov 13, 2023

pichlermarc left a comment

pichlermarc Nov 13, 2023

legendecas commented Nov 29, 2023

pichlermarc commented Nov 29, 2023 •

edited

Loading

trentm Nov 29, 2023

trentm Nov 29, 2023

legendecas Nov 29, 2023 •

edited

Loading

trentm commented Dec 1, 2023

ci: add npm cache in actions/setup-node #4271

ci: add npm cache in actions/setup-node #4271

Conversation

lyzlisa commented Nov 8, 2023 • edited Loading

Which problem is this PR solving?

Short description of the changes

Type of change

How Has This Been Tested?

Checklist:

linux-foundation-easycla bot commented Nov 8, 2023 • edited Loading

codecov bot commented Nov 10, 2023 • edited Loading

Codecov Report

lyzlisa Nov 10, 2023

Choose a reason for hiding this comment

pichlermarc Nov 13, 2023

Choose a reason for hiding this comment

pichlermarc left a comment

Choose a reason for hiding this comment

pichlermarc Nov 13, 2023

Choose a reason for hiding this comment

legendecas commented Nov 29, 2023

pichlermarc commented Nov 29, 2023 • edited Loading

trentm Nov 29, 2023

Choose a reason for hiding this comment

trentm Nov 29, 2023

Choose a reason for hiding this comment

legendecas Nov 29, 2023 • edited Loading

Choose a reason for hiding this comment

trentm commented Dec 1, 2023

lyzlisa commented Nov 8, 2023 •

edited

Loading

linux-foundation-easycla bot commented Nov 8, 2023 •

edited

Loading

codecov bot commented Nov 10, 2023 •

edited

Loading

pichlermarc commented Nov 29, 2023 •

edited

Loading

legendecas Nov 29, 2023 •

edited

Loading