Fix deploy using client-id and client-secret #2820
Conversation
ghost
assigned 
CecileRobertMichon
changed the title
cmd/deploy.go
Outdated
| @@ -241,6 +241,11 @@ func autofillApimodel(dc *deployCmd) { | |||
| Secret: secret, | |||
| ObjectID: servicePrincipalObjectID, | |||
| } | |||
| } else if dc.containerService.Properties.ServicePrincipalProfile == nil && dc.ClientID.String() != "" && dc.ClientSecret != "" { | |||
There was a problem hiding this comment.
what about the case where dc.containerService.Properties.ServicePrincipalProfile is not nil but is empty? For example, "servicePrincipalProfile": { "clientId": "", "secret": "" }
There was a problem hiding this comment.
We need to address this case since it applies to most example json apimodel in the examples/ folder
| @@ -40,6 +41,19 @@ const ExampleAPIModelWithDNSPrefix = `{ | |||
| } | |||
| ` | |||
|
|
|||
| const ExampleAPIModelWithoutServicePrincipalProfile = `{ | |||
There was a problem hiding this comment.
Let's also add a test case for empty ServicePrincipalProfile
There was a problem hiding this comment.
@CecileRobertMichon A couple of questions
- just wanted to double check the expected result.
If ServicePrincipalProfile is Empty, and no client id and secret specified in deployment command (--client-id , --client-secret) then expected result should be
- deployCmd.containerService.Properties.ServicePrincipalProfile.ClientID == ""
- deployCmd.containerService.Properties.ServicePrincipalProfile.Secret == ""
?
- If ServicePrincipalProfile is Empty, and client id and secret are specified in deployment command (--client-id , --client-secret) then should the code use the client-id and client-secret in the deployment command? This second part will need modification to code as well as new test cases. If this is to be implemented perhaps it would make more sense to have a separate new issue
There was a problem hiding this comment.
- That's correct
- Yes, exactly. It shouldn't be very complicated to add to this PR. I believe all you need to do is to change your if statement in deploy.go to
} else if (dc.containerService.Properties.ServicePrincipalProfile == nil || (dc.containerService.Properties.ServicePrincipalProfile.ClientID == "" && dc.containerService.Properties.ServicePrincipalProfile.Secret == "") ) && dc.ClientID.String() != "" && dc.ClientSecret != ""and add the appropriate unit test case. We just want to make sure that the change you made applies if the sp profile is declared but contains no ID and secret values.
There was a problem hiding this comment.
Thanks. Makes sense. So test cases for conditions where id and secret are specified in both the api model and deployment command can be taken up later.
There was a problem hiding this comment.
@CecileRobertMichon i have made the requested changes, please review and let me know if any further changes are needed
Codecov Report
@@ Coverage Diff @@
## master #2820 +/- ##
==========================================
+ Coverage 46.94% 46.94% +<.01%
==========================================
Files 86 86
Lines 12809 12813 +4
==========================================
+ Hits 6013 6015 +2
- Misses 6241 6242 +1
- Partials 555 556 +1
Continue to review full report at Codecov.
|
What this PR does / why we need it: Fix Issue #2657
Which issue this PR fixes (optional, in
fixes #2657format, will close that issue when PR gets merged): fixes #2657#2657Special notes for your reviewer:
If applicable:
Release note: