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

fix(upgrade): Keep lockfile if no pkgs change #1671

Merged
merged 1 commit into from
Jun 21, 2024

Conversation

dcarley
Copy link
Contributor

@dcarley dcarley commented Jun 21, 2024

Proposed Changes

It's possible for the catalog server to resolve the same package version and derivation from a different page and revision of nixpkgs.

In these cases the packages haven't actually changed and we would tell the user that they haven't been upgraded, however we were still updating the lockfile with the new page, rev, rev_count, and locked_url.

Prevent this from happening by returning the existing lockfile. We currently still trigger a build on it though, in part because an existing test expects a noop upgrade to generate a lockfile and the outer function doesn't know whether the lockfile has changed.

A side-effect of this is that the test can't compare the raw lockfile contents because it appears to originally be written out in pretty-print format and then written in compact format, so we have to parse it with jq to do the comparison.

I did experiment with making this a unit test instead of an integration test, in part because the mock manipulation feels quite fragile, but the round-trip behaviour wasn't as clear and it likely wouldn't have highlighted the oddities above.

I also considered moving the mock manipulation into test_data but decided against it because it's clearer inline to understand the one test that uses it.

Release Notes

N/A, part of catalog release.

It's possible for the catalog server to resolve the same package version
and derivation from a different page and revision of nixpkgs.

In these cases the packages haven't actually changed and we would tell
the user that they haven't been upgraded, however we were still updating
the lockfile with the new `page`, `rev`, `rev_count`, and `locked_url`.

Prevent this from happening by returning the existing lockfile. We
currently still trigger a build on it though, in part because an
existing test expects a noop upgrade to generate a lockfile and the
outer function doesn't know whether the lockfile has changed.

A side-effect of this is that the test can't compare the raw lockfile
contents because it appears to originally be written out in pretty-print
format and then written in compact format, so we have to parse it with
`jq` to do the comparison.

I did experiment with making this a unit test instead of an integration
test, in part because the mock manipulation feels quite fragile, but the
round-trip behaviour wasn't as clear and it likely wouldn't have
highlighted the oddities above.

I also considered moving the mock manipulation into `test_data` but
decided against it because it's clearer inline to understand the one
test that uses it.
@dcarley dcarley requested a review from a team as a code owner June 21, 2024 12:02
@billlevine
Copy link
Contributor

I think this makes sense, but I'm wondering if there is a reason to advance the page anyway? I don't know of any such reason, but I do know that there have been nix-based considerations around advancing pages that were not immediately intuitive for me at least.

@zmitchell zmitchell added this pull request to the merge queue Jun 21, 2024
Merged via the queue into main with commit 6753982 Jun 21, 2024
15 checks passed
@zmitchell zmitchell deleted the dcarley/1570-upgrade-same-drv-different-rev branch June 21, 2024 18:09
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.

None yet

3 participants