omdb nexus blueprints target set --diff
#6435
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Tested against First, we need a new blueprint: Here's using the diff and aborting: Here's using the diff and confirming: Here's without using it: Since the code path around Here's the same situation but where the parent was enabled: |
jgallagher
approved these changes
Aug 26, 2024
dev-tools/omdb/src/bin/omdb/nexus.rs
Outdated
| @@ -1722,6 +1726,40 @@ async fn cmd_nexus_blueprints_target_set( | |||
| args: &BlueprintTargetSetArgs, | |||
| _destruction_token: DestructiveOperationToken, | |||
| ) -> Result<(), anyhow::Error> { | |||
| let found_enabled = if args.diff | |||
| || matches!(args.enabled, BlueprintTargetSetEnabled::Inherit) | |||
There was a problem hiding this comment.
This (and the unwrap) seem kinda awkward. Would it be too clever to stick "get the current target" in a tokio OnceCell? Something like
// Helper to only fetch the current target once. We may need it immediately
// if `args.diff` is true, or later if `args.enabled` is "inherit" (or
// both).
let current_target = OnceCell::new();
let get_current_target = || async {
current_target
.get_or_try_init(|| client.blueprint_target_view())
.await
.context("failed to fetch current target blueprint")
};at which point we could drop the check for args.enabled entirely here:
- let found_enabled = if args.diff
- || matches!(args.enabled, BlueprintTargetSetEnabled::Inherit)
- {
- let current_target = client
- .blueprint_target_view()
- .await
- .context("failed to fetch current target blueprint")?
- .into_inner();
-
if args.diff {
+ let current_target = get_current_target().await?;
let blueprint1 = client
.blueprint_view(¤t_target.target_id)
.await
@@ -1755,11 +1748,6 @@ async fn cmd_nexus_blueprints_target_set(
prompt.read_and_validate("y/N", "y")?;
}
- Some(current_target.enabled)
- } else {
- None
- };
-
let enabled = match args.enabled {
BlueprintTargetSetEnabled::Enabled => true,
BlueprintTargetSetEnabled::Disabled => false,
@@ -1772,9 +1760,9 @@ async fn cmd_nexus_blueprints_target_set(
// operator. (In the case of the current target blueprint being changed
// entirely, that will result in a failure to set the current target
// below, because its parent will no longer be the current target.)
- //
- // unwrap(): this is always `Some` due to the branch above.
- BlueprintTargetSetEnabled::Inherit => found_enabled.unwrap(),
+ BlueprintTargetSetEnabled::Inherit => {
+ get_current_target().await?.enabled
+ }There was a problem hiding this comment.
Agreed. I tried and failed to do this before. I'll take a look at that diff and see if I can make it work.
There was a problem hiding this comment.
Great! I like this much better. Done in f9beb4c.
|
Testing on f9beb4c. Set up: First, verify you can just set a target with "inherit" the same as before: Same, but this time enable the original target first: Now with diff, but abort: Now, with diff and confirm: |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This adds a
--diffflag toomdb nexus blueprints target setthat presents a diff against the current target blueprint and then prompts for confirmation before proceeding.