-
Notifications
You must be signed in to change notification settings - Fork 152
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
Changes to support Secret Credential type for azure #1275
Conversation
Merge branch 'master' of https://github.com/kanisterio/kanister into profile-secret-change
Merge branch 'master' of https://github.com/kanisterio/kanister into profile-secret-change
Merge branch 'master' of https://github.com/kanisterio/kanister into profile-secret-change
Merge branch 'master' of https://github.com/kanisterio/kanister into profile-secret-change
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Couple of minor comments.
@@ -234,26 +233,59 @@ func getLocationParams(cmd *cobra.Command) (*locationParams, error) { | |||
}, nil | |||
} | |||
|
|||
func constructProfile(lP *locationParams, secret *v1.Secret, role string) *v1alpha1.Profile { | |||
func constructProfile(lP *locationParams, secret *v1.Secret) *v1alpha1.Profile { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I noticed that there are no unit tests for testing either constructProfile/constructSecret or even the caller createNewProfile(used in newAzureProfileCmd/newGCPProfileCmd/newS3CompliantProfileCmd). It would be useful to add tests using fake kube clients.
retCredType: v1alpha1.CredentialTypeKeyPair, | ||
}, | ||
} { | ||
prof := constructProfile(tc.lp, tc.secret) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah nice. I read the test file after adding my comment about testing in profile.go. Happy to see that you have already added the test 👍🏼 .
pkg/secrets/azure.go
Outdated
azSecret.EnvironmentName = string(envName) | ||
} | ||
if azSecret.StorageAccount == "" || azSecret.StorageKey == "" { | ||
return nil, errors.New("Azure secret is missing storage account name or storage key") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return nil, errors.New("Azure secret is missing storage account name or storage key") | |
return nil, errors.New("Azure secret is missing storage account ID or storage key") |
pkg/validate/validate.go
Outdated
// The following is KeyPair codepath | ||
kp := p.Credential.KeyPair | ||
if kp == nil { | ||
return nil, errorf(validateErr, "Invalid credentials kv cannot be nil") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return nil, errorf(validateErr, "Invalid credentials kv cannot be nil") | |
return nil, errorf(validateErr, "Invalid credentials kp cannot be nil") |
@bathina2 Removing |
Change Overview
The following PR enables Secret Credetial type for azure. This allows us to pass the azure cloud environment down as an argument in the secret.
This PR also uses an updated stow version.
Pull request type
Please check the type of change your PR introduces:
Issues
Test Plan