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

chore(pre-commit): test:prep on package-lock.json change #987

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

Conversation

legobeat
Copy link
Contributor

@legobeat legobeat commented Jan 31, 2024

Changes in package-lock.json often result in fixture policy changes.

This runs test:prep as part of git pre-commit hook if lockfile is changed.

Related

@legobeat legobeat marked this pull request as ready for review January 31, 2024 22:48
@legobeat legobeat requested a review from a team as a code owner January 31, 2024 22:48
@legobeat legobeat added the chore overhead, tests, dev env, etc. label Jan 31, 2024
Copy link
Contributor

@boneskull boneskull left a comment

Choose a reason for hiding this comment

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

This won't actually add any changed policy files to the commit, if that's what you wanted to happen. I don't think we want to do that, do we? This also won't tell the developer that "hey, some policy files changed..." until next time they run git status. By then, it might be running in CI already, and breaking because files changed in VCS (eventually; #825).

I feel like we might want to take a step back and ask why we keep updating these policy files in the first place.

Why would a change to our package-lock.json cause the policy in a fixture to update? Are the tests (usually this is browserify) resolving things they shouldn't? It doesn't seem like proper isolation.

@boneskull
Copy link
Contributor

I guess I see why the file in #988 gets updated; that seems to have to do with hoisting. Still, I can't shake the feeling that the policy gen should not be so sensitive to the structure of node_modules. Maybe it's not really possible to do it any other way, or maybe it's like that for a reason that I don't understand.

@legobeat
Copy link
Contributor Author

legobeat commented Feb 1, 2024

This won't actually add any changed policy files to the commit, if that's what you wanted to happen. I don't think we want to do that, do we?

Oh, this is indeed what I was aiming for. Either that or throwing in case of changes. How to get there?

And yeah, for now I think we need to keep these synced in git, despite the churn.

@boneskull
Copy link
Contributor

To add, you'd have to do && git -A <files-expected-to-change>

@legobeat legobeat marked this pull request as draft February 3, 2024 11:11
@boneskull
Copy link
Contributor

@legobeat

And yeah, for now I think we need to keep these synced in git, despite the churn.

Why?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
chore overhead, tests, dev env, etc.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants