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

Use script to install node dependencies to benefit from custom cache handling #166

Merged
merged 1 commit into from
May 24, 2024

Conversation

wojtekn
Copy link
Contributor

@wojtekn wojtekn commented May 24, 2024

Proposed Changes

I propose to use a script to install node dependencies to benefit from custom cache handling, similar to what we do in other steps.

Testing Instructions

Merge and confirm distribution job works fine.

Pre-merge Checklist

  • Have you checked for TypeScript, React or other console errors?

@wojtekn wojtekn requested a review from a team May 24, 2024 16:03
@wojtekn wojtekn self-assigned this May 24, 2024
install_npm_packages
.buildkite/commands/install-node-dependencies.sh
Copy link
Contributor

Choose a reason for hiding this comment

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

The install_npm_packages is already a script that handles caching, defined in our a8c-ci-toolkit plugin.

Would it make sense to add the handling of patches/ in the CACHE_KEY… in the script from the plugin, so that other projects can benefit from it too? Or is the handling of patches/* and also what install-node-dependencies.sh does is too specific to studio to be worth fixing upstream too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It makes sense, but as four steps use custom install-node-dependencies.sh and two steps use a more generic command from upstream, I would prefer to fix it by using the consistent custom script now and bringing those changes to upstream separately if it benefits other projects that use that.

The number of changes is bigger than cache key handling e.g. one uses npm ci vs npm install.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@wojtekn wojtekn merged commit 9e10b59 into trunk May 24, 2024
11 checks passed
@wojtekn wojtekn deleted the fix/installing-node-dependencies branch May 24, 2024 16:17
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.

2 participants