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(ci): better check for Nocks changes #491

Merged
merged 2 commits into from
Jun 6, 2024
Merged

chore(ci): better check for Nocks changes #491

merged 2 commits into from
Jun 6, 2024

Conversation

aduh95
Copy link
Contributor

@aduh95 aduh95 commented Jun 6, 2024

When there are no changes, asking Git to compare the db file directly will always report change even though the actual data stored is the same. Instead, we can compare a SQL dump to detect actual changes.

When there are no changes, asking Git to compare the db file directly
will always report change even though the actual data stored is the
same. Instead, we can compare a SQL dump to detect actual changes.
@aduh95 aduh95 requested a review from merceyz June 6, 2024 08:39
Comment on lines +55 to +56
cp tests/nocks.db tests/nocks.db.new
git checkout HEAD -- tests/nocks.db
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Rather than the cp, you could use git show to dump the old content in a /tmp file without mutating the real nocks.db.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've considered it, and as I was not sure what would be the behavior of git show with binary files, I decided to do that cp dance instead. I don't feel strongly either way, happy to change it if you think it's better

Copy link
Contributor

@arcanis arcanis Jun 6, 2024

Choose a reason for hiding this comment

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

I'd expect it to be possible but I can't find the right syntax locally (I get no output), so feel free to merge as-is.

@aduh95
Copy link
Contributor Author

aduh95 commented Jun 6, 2024

@aduh95 aduh95 enabled auto-merge (squash) June 6, 2024 12:11
@aduh95 aduh95 merged commit 3da5346 into main Jun 6, 2024
10 of 11 checks passed
@aduh95 aduh95 deleted the better-checks branch June 6, 2024 12:20
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.

3 participants