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

Update pinned maghemite #5726

Merged
merged 5 commits into from May 12, 2024
Merged

Update pinned maghemite #5726

merged 5 commits into from May 12, 2024

Conversation

charliepark
Copy link
Contributor

@charliepark charliepark commented May 9, 2024

The pinned sha for maghemite was out of date, pointing to an invalid sha (I believe because of some force pushes that removed the specific commit).

This updates omicron to point to the most recent sha for maghemite and updates the necessary checksums so the tests pass.

Some context on why this is even needed: because there is no mac build of mgd, we build it from source for macs, and need the correct sha for that.

@charliepark
Copy link
Contributor Author

I'm not totally confident there isn't a step I've missed in this; if @rcgoodfellow or others have things to point out, I'm all ears.

@david-crespo
Copy link
Contributor

Heh just caught this. It would be nice if either this mismatch could be caught in CI and/or it was easier to make the change.

# Updating the commit hash here currently requires also updating
# `tools/maghemite_openapi_version`. Failing to do so will cause a failure when
# building `ddm-admin-client` (which will instruct you to update
# `tools/maghemite_openapi_version`).
source.commit = "025389ff39d594bf2b815377e2c1dc4dd23b1f96"

@david-crespo
Copy link
Contributor

david-crespo commented May 9, 2024

Since it seems like package-manifest.toml is more likely to be correct, I updated the hack script we use to pull the dendrite and maghemite commits from there. But this mismatch will probably affect someone else eventually. The nix flake uses the commit from the files changed in this PR, though because it can pull the linux binary, it will at least be able to find a file on buildomat. However, it may not match the binary we are actually shipping on a given omicron commit, so that's probably bad.

https://gist.github.com/david-crespo/d8aa7ea1afb877ff585e6ad90fc5bc2c

Copy link
Contributor

@rcgoodfellow rcgoodfellow left a comment

Choose a reason for hiding this comment

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

Thanks for fixing this. Ultimately it would be nice to get rid of these environment variable files and just use what's in Cargo.toml.

@david-crespo david-crespo merged commit 3641a15 into main May 12, 2024
20 checks passed
@david-crespo david-crespo deleted the update-pinned-maghemite branch May 12, 2024 14:02
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