az cli client cleanup #434
Conversation
davidgamero
commented
Nov 22, 2024
•
davidgamero
commented
- move az cli methods to within the azClient
- consume CommandRunner for az cli
- update Ensure/Validation for AzCLI to methods that return errors so we can consume and test them directly via fakeCommandRunners
- revert contributor role to allow acr build and push for github action
| @@ -34,7 +34,6 @@ application and service principle, and will configure that application to trust | |||
| ctx := cmd.Context() | |||
|
|
|||
| gh := providers.NewGhClient() | |||
| providers.EnsureAzCli() | |||
There was a problem hiding this comment.
moved this to inside the az client constructor
| var _ CommandRunner = &DefaultCommandRunner{} | ||
|
|
||
| func (d *DefaultCommandRunner) RunCommand(args ...string) (string, error) { | ||
| log.Debug("Running command: ", args) |
There was a problem hiding this comment.
add debug log here to allow tracing command executions while debugging
| @@ -10,10 +10,6 @@ deployVariables: | |||
| value: "testapp" | |||
| - name: "IMAGENAME" | |||
There was a problem hiding this comment.
we should rework the integration test generation to no longer be a big bash script and maybe leverage go natively. then we can integrate better defaults per-language
| @@ -141,28 +142,10 @@ func NoBlankStringValidator(s string) error { | |||
|
|
|||
| // Validator for App name | |||
| func appNameValidator(name string) error { | |||
| if name == "" { | |||
| return fmt.Errorf("application name cannot be empty") | |||
| errors := validation.IsDNS1123Label(name) | |||
There was a problem hiding this comment.
💡 Note only please for archive sake: IsDNS1123Label tests for a string that conforms to the definition of a label in DNS (RFC 1123).
| for _, r := range name { | ||
| if unicode.IsLetter(r) || unicode.IsDigit(r) || r == '-' || r == '_' || r == '.' { | ||
| if unicode.IsLetter(r) || unicode.IsDigit(r) || r == '-' || r == '.' { |
There was a problem hiding this comment.
💡 Suggestions only please:
validChars := unicode.IsLetter(r) || unicode.IsDigit(r) || strings.ContainsRune("-.", r)
if validChars {
builder.WriteRune(r)
}| if err != nil { | ||
| log.Fatalf("Error validating az cli installation: %s", err.Error()) | ||
| } | ||
| az.EnsureAzCliLoggedIn() |
There was a problem hiding this comment.
💡 Possible suggestion:
func (az *AzClient) EnsureAzCli() error {
if err := az.ValidateAzCliInstalled(); err != nil {
return fmt.Errorf("failed to validate az CLI installation: %w", err)
}
if err := az.EnsureAzCliLoggedIn(); err != nil {
return fmt.Errorf("failed to ensure az CLI login: %w", err)
}
return nil
}| } | ||
| log.Debug("Service principal created successfully!") |
There was a problem hiding this comment.
💡 Suggestion only please: Use case suggestion: since we create SP for the user, I hope we have agreement from user and informed them about the age of SP and reason it was created and how user can destruct if need be.
Tatsinnit
requested review from
bfoley13,
Vidya2606,
aamgayle,
erinborders and
bosesuneha
