-
Notifications
You must be signed in to change notification settings - Fork 204
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
saving cosmosdb keys as secrets #935
Conversation
|
||
result, err := client.ListKeys(ctx, groupName, accountName) | ||
if err != nil { | ||
return nil, errhelp.NewAzureErrorAzureError(err) |
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.
normally we just return the error and convert it to an AzureError on the other side...any reason to deviate here?
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 originally did this for all of the AzureCosmosDBManager functions because it would simplify the Ensure function which tend to grow pretty large. I can change it to pass just an error if that's preferrable?
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 think it would be OK as a proposed change to all operators but not a good idea for just this one operator.
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.
The best thing you can do to clean up the Ensure function would be to reduce the cyclomatic complexity.
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 re-wrote most of the ensure function. I was able to reduce the cyclomatic complexity until I realized I wasn't handling a few error cases so it is about the same as before. I'll let you resolve this one when you feel like it's addressed.
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.
some initial comments/questions
/azp test |
Command 'test' is not supported by Azure Pipelines. Supported commands
See additional documentation. |
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
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
Closes #879
What this PR does / why we need it:
Saves the connection strings and account keys as secrets.
Special notes for your reviewer:
How does this PR make you feel:
If applicable: