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

Split lint into check and build, switch from npm install to npm ci #315

Merged
merged 1 commit into from
Nov 24, 2023

Conversation

dbast
Copy link
Member

@dbast dbast commented Nov 23, 2023

This splits the lint flow into two jobs:

  • one only calling npm run check, which can just reveal some unrelated formatting changes missing. Those findings still have to be fixed, but in most cases have no consequences in regards to if the example workflow runs are all valid or not.
  • one calling npm run build and checking if there are no updates = all updates in the ./dist folder are committed... If this job fails, then all the examples runs have to be taken with a grain of salt, as they run agains an outdated ./dist folder, thus being invalid!

This information was previously not visible / masked as mostly npm run check failed and prevented npm run build from running ... it was then unclear, if that has consequences to the ncc compiled stuff in ./dist or not.

Also switching npm install to npm ci to install the exact versions from package-lock.json (managed by dependabot). This is faster, more secure and 100% predictable. Thus removing the cron triggers as those won't find anything new.

@dbast dbast requested a review from a team as a code owner November 23, 2023 16:20
@dbast
Copy link
Member Author

dbast commented Nov 23, 2023

yay... lint and build each only take 14 sec.

npm install
# ci: install exact versions from package-lock.json
# fast, secure, predictable compared to npm install
npm ci
Copy link
Member

@goanpeca goanpeca Nov 23, 2023

Choose a reason for hiding this comment

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

Should we test both then... npm ci and npm install ?

Or is there no point 🙃 ?

Copy link
Member Author

Choose a reason for hiding this comment

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

well, there is no point in doing so... the package-lock.json is basically the solved result of npm install and contains all dependencies + transitive dependencies and their sha512 checksums.... but npm is not the subject under test here... we want to get all the dependencies + dev dependencies installed in a predictable+secure way (the hashes are verified) to then run linters / build checks... thats exactly what npm ci exists for ... dependabot keeps the package.json + package-json.lock updated / in sync for us.

Copy link
Member

Choose a reason for hiding this comment

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

Got it :)

@goanpeca
Copy link
Member

Awesome, thanks for working on this 🚀

@goanpeca goanpeca merged commit 6401a9e into conda-incubator:main Nov 24, 2023
50 checks passed
@dbast dbast deleted the split-lint branch November 25, 2023 08:14
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