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

ci: check for unstaged files #1685

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Conversation

roschaefer
Copy link
Collaborator

@roschaefer roschaefer commented Aug 17, 2023

When you do the following

$ asdf install
$ npm --version
6.14.16

and run:

npm install

then you get a changed package-lock.json.

It is good practice to always check in lock files into version control.
On CI we run npm install (among other things). If we make sure that we
do not leave any updates files, this ensures we always commit our lock
files.

EDIT: You don't, my fault. But it does not hurt to merge this PR anyways.

@roschaefer roschaefer requested a review from mattwr18 August 17, 2023 13:11
@roschaefer roschaefer force-pushed the ci/check-unstaged-changes branch 3 times, most recently from 68a00cc to c3544d7 Compare August 17, 2023 14:14
.tool-versions Outdated
@@ -1,3 +1,3 @@
ruby 3.0.0
nodejs lts-erbium
nodejs 12.22.12
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

So this was necessary for the Github action marocchino/tool-versions-action to work. However it is now considered bad practice to set codename versions: https://github.com/asdf-vm/asdf-nodejs#partial-and-codename-versions

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

$ asdf nodejs resolve lts-erbium --latest-available
12.22.12

roschaefer added a commit that referenced this pull request Sep 5, 2023
Motivation
----------
Follow-up of #1685 @mattwr18 mentioned it's doing too many things.

How to test
-----------
1. Check if CI fails
When you do the following
```
$ asdf install
$ npm --version
6.14.16
```

and run:

```
npm install
```

~then you get a changed `package-lock.json`.~

It is good practice to always check in lock files into version control.
On CI we run `npm install` (among other things). If we make sure that we
do not leave any updates files, this ensures we always commit our lock
files.

EDIT: You don't, my fault. But it does not hurt to merge this PR anyways.
@roschaefer roschaefer force-pushed the ci/check-unstaged-changes branch from c3544d7 to d6216db Compare September 5, 2023 15:13
Copy link
Contributor

@mattwr18 mattwr18 left a comment

Choose a reason for hiding this comment

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

I have to admit I don't understand what the purpose of these changes are.
Could you please update the description as it is now outdated and it seems has nothing to do with these changes.

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