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

Add split-receipt command that splits receipt into phase 1 and 2 uninstallation flows #1278

Merged
merged 19 commits into from
Nov 12, 2024

Conversation

cole-h
Copy link
Member

@cole-h cole-h commented Nov 8, 2024

Description

Depends on #1277.

This will make it easier to reinstall Nix with a newer version of the nix-installer (which, as it happens, also makes it easier to install Determinate on top of an existing Nix installation without having to completely nuke your /nix).

Checklist
  • Formatted with cargo fmt
  • Built with nix build
  • Ran flake checks with nix flake check
  • Added or updated relevant tests (leave unchecked if not applicable)
  • Added or updated relevant documentation (leave unchecked if not applicable)
  • Linked to related issues (leave unchecked if not applicable)
Validating with install.determinate.systems

If a maintainer has added the upload to s3 label to this PR, it will become available for installation via install.determinate.systems:

curl --proto '=https' --tlsv1.2 -sSf -L https://install.determinate.systems/nix/pr/$PR_NUMBER | sh -s -- install

TODO:

  • regenerate fixtures
  • test this flow in a vm
  • other things?

@cole-h cole-h added the upload to s3 The labeled PR is allowed to upload its artifacts to S3 for easy testing label Nov 8, 2024
Base automatically changed from phased-uninstall-related to main November 8, 2024 19:51
@cole-h cole-h added this to the 0.28.0 milestone Nov 8, 2024
@grahamc grahamc marked this pull request as ready for review November 9, 2024 03:13
Comment on lines 107 to 115
match serde_json::from_str::<InstallPlan>(&install_receipt_string)
.ok()
.and_then(|plan| {
if plan.check_compatible().is_ok() {
Some(plan)
} else {
None
}
}) {
Copy link
Member

Choose a reason for hiding this comment

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

nit: not a huge fan of non-trivial expressions as match statement, would be nice to see this split out in a separate let, then match.

Copy link
Member

Choose a reason for hiding this comment

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

addressed in e43ca23

Comment on lines 469 to 477
let action_json = serde_json::to_string(action).with_context(|| {
format!("serde_json::to_string'ing {action_tag} json to extract real type")
})?;
let action_unjson: StatefulAction<T> =
serde_json::from_str(&action_json).with_context(|| {
format!("serde_json::from_str'ing {action_tag} json to extract real type")
})?;

Ok(action_unjson)
Copy link
Member

Choose a reason for hiding this comment

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

nit: Is there no way to get action_tag from T? It seems weird that we pass the type-to-deserialize to as a Type (T) and also pass action_tag. I was surprised to see that, but the understand it's just used for the log message. It feels redundant though if there's some compile/runtime-way to get a human readable name of T.

Copy link
Member

Choose a reason for hiding this comment

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

addressed in e5ce505

@grahamc grahamc merged commit 2e04848 into main Nov 12, 2024
21 checks passed
@grahamc grahamc deleted the phased-uninstall branch November 12, 2024 22:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
upload to s3 The labeled PR is allowed to upload its artifacts to S3 for easy testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants