rikhuijzer / cache-install

Use the GitHub Actions cache for Nix packages
MIT License
42 stars 19 forks source link

Remove `node_modules` #7

Closed mbg closed 2 years ago

mbg commented 2 years ago

The node_modules folder should not be checked into version control. I have removed it and made changes to the action/CI to accommodate for this, which are modelled after GitHub's template for JavaScript actions.

Specifically:

rikhuijzer commented 2 years ago

Thanks for taking a look at the repo.

The node_modules folder should not be checked into version control.

In my defense, this was the default way of doing things when I created this repository. And, the good news is that the repo worked even though I haven't touched it in over a year. Just to check, the package-lock.json is used to reproduce the versions?

mbg commented 2 years ago

Just to check, the package-lock.json is used to reproduce the versions?

Yep, at least that's npm's intention. Thanks for the very quick response!

rikhuijzer commented 2 years ago

Can you remove the - mbg/improvements from test.yml? It could be a bit confusing to future readers. If you want to run CI on your fork, my tip is to create a PR inside the fork from the changed branch to main/master. Then, CI will run there. Once everything looks good, close the PR inside the fork and create a fork to the main repo. EDIT: without closing the PR in the fork, the GitHub interface doesn't allow creating a PR against the original repo, but of course other tooling should work

Finally, could you please explain a bit more about the check? It's probably good, but I don't understand yet what it does :)

mbg commented 2 years ago

Can you remove the - mbg/improvements from test.yml?

Oops, my mistake, I thought I had removed that already. It's gone now!

Finally, could you please explain a bit more about the check? It's probably good, but I don't understand yet what it does :)

The check is more or less identical to the one for the JavaScript template. It installs Node+npm, then uses npm ci to install the dependencies according to package-lock.json, and then rebuilds the contents of the dist/ folder using npm run package. The "Compare the expected and actual dist/ directories" step uses git diff to check whether running npm run package has produced different files to the ones that are already in the repository. If not, it means that the contents of dist/ are up-to-date and the workflow succeeds. If they are different, the last step runs which uploads the contents of dist/ as a build artifact and the workflow fails.

rikhuijzer commented 2 years ago

We can by the way probably simplify this project a lot because there is now a composite feature for GitHub Actions. For example, we've used it at https://github.com/julia-actions/cache/blob/main/action.yml to wrap around the GitHub Cache Action.

mbg commented 2 years ago

Looks good. I gave you access to this repo so you can merge when you're happy

Thanks, although that wasn't necessary!

We can by the way probably simplify this project a lot because there is now a composite feature for GitHub Actions.

True. I think the main value of this action is in the core.sh script. You could probably move that into a composite action, preceded by actions/cache.

rikhuijzer commented 1 year ago

True. I think the main value of this action is in the core.sh script. You could probably move that into a composite action, preceded by actions/cache.

Implemented now with: