Fix alias import clobber flag by samcoe · Pull Request #7569 · cli/cli

samcoe · 2023-06-12T00:01:38Z

Nested aliases broke this functionality as we were basically ignoring the flag and never reached the code the checked it, this PR restores it back to its previous functionality.

Fixes #7565

samcoe · 2023-06-12T00:03:08Z

pkg/cmd/alias/imports/import_test.go

+			for _, cmd := range tt.aliasCommands {
+				rootCmd.AddCommand(cmd)
+			}


All the tests were previously passing because these alias commands were not set up properly.

williammartin

The looks functionally correct. For what it's worth, I spent a bit of time going back and forth understanding the relationship between existingAlias and validAliasName and I'm still not sure I totally get it.

My understanding is that validAliasName will now return false because our alias is shadowing an existing one that we now attach to our command tree, but then we use our knowledge of the aliasCfg to decide why it wasn't valid.

However, it doesn't really seem like we need to know if it the alias was valid before indicating failure, and that 141-149 could be hoisted up out of the conditional and the conditional on line 131 can be removed again.

Alternatively, we could move the existingAlias logic into our opts.ValidAliasName, injecting the aliasCfg and have it provide more useful information in the return type.

I guess at the core of it, I think the relationship between validity of an alias name and whether an alias already exists could be made clearer, but I understand that's subjective so I've approved this anyway.

samcoe · 2023-06-20T00:21:27Z

@williammartin Thanks for the review. I can understand where you are coming from where existingAlias and validAliasName have a confusing relationship. Your understanding of what is happening is accurate though. validAliasName has a number of checks around what makes an alias name valid including checking if there is already an command (including aliases) with the same name. When using the --clobber flag of the alias import command we want to still run all of those validations but ignore the one checking if there is an alias with the same name. validAliasName does not return why an alias name is invalid though hence why there is the secondary existingAlias check. I thought about changing validAliasName to return a reason but that would have resulted in changing a couple other places where we validate aliases. This approach seemed like the smallest change to fix the issue. I am happy to add some comments into the code in a separate PR if you feel they would make this more clear.

Fix alias import clobber flag

5b17925

samcoe requested a review from a team as a code owner June 12, 2023 00:01

samcoe self-assigned this Jun 12, 2023

samcoe requested review from vilmibm and removed request for a team June 12, 2023 00:01

samcoe commented Jun 12, 2023

View reviewed changes

williammartin approved these changes Jun 19, 2023

View reviewed changes

samcoe merged commit 78839db into trunk Jun 20, 2023

samcoe deleted the fix-alias-import-clobber branch June 20, 2023 00:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix alias import clobber flag#7569

Fix alias import clobber flag#7569
samcoe merged 1 commit intotrunkfrom
fix-alias-import-clobber

samcoe commented Jun 12, 2023

Uh oh!

samcoe Jun 12, 2023

Uh oh!

williammartin left a comment •

edited

Loading

Uh oh!

samcoe commented Jun 20, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

samcoe commented Jun 12, 2023

Uh oh!

samcoe Jun 12, 2023

Choose a reason for hiding this comment

Uh oh!

williammartin left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

samcoe commented Jun 20, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

williammartin left a comment •

edited

Loading