-
Notifications
You must be signed in to change notification settings - Fork 5.2k
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
Change connection-string to be object and support making severname, d… #2834
Conversation
…atabasename visible to customers
Automation for azure-sdk-for-pythonThe initial PR has been merged into your service PR: |
Automation for azure-libraries-for-javaThe initial PR has been merged into your service PR: |
Automation for azure-sdk-for-goA PR has been created for you based on this PR content. Once this PR will be merged, content will be added to your service PR: |
Automation for azure-sdk-for-nodeThe initial PR has been merged into your service PR: |
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 descriptions should really be updated to mention that there is a non-trivial string type.
@@ -104,7 +104,7 @@ | |||
"properties": { | |||
"connectionString": { | |||
"description": "The connection string.", | |||
"$ref": "../datafactory.json#/definitions/SecretBase" | |||
"type": "object" |
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 see SecretBase
was an object
with a property type
. Does that continue to be true here? Reading the description without the context of this PR, I would be surprised if this was anything but a string
.
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.
Before the connectionString was defined as SecretBase, and customers can only give value as "SecureString" or "AKV" (Two data contract used in ADF) for this field. Now we want to support customer to provide value with string directly while the existed "SecureString" and "AKV" can still work, so we change its type to be object.
"type": "object" | ||
}, | ||
"encryptedCredential": { | ||
"description": "The encrypted credential used for authentication. Credentials are encrypted using the integration runtime credential manager. Type: string (or Expression with resultType string).", |
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, I'm presuming that this is the answer to my question above. It would be worth adding that to the description, if it's the case.
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.
Description is updated.
Ah, another thing that I should point out @zhanyu2014 is that it says your CLA hasn't been signed. I assume you're a Microsoft employee, so you should go to http://opensource and join the appropriate organizations. |
Adding the ARM review label before I forget. |
@AutorestCI regenerate azure-sdk-for-go |
Hi There, I am the AutoRest Linter Azure bot. I am here to help. My task is to analyze the situation from the AutoRest linter perspective. Please review the below analysis result: File: AutoRest Linter Guidelines | AutoRest Linter Issues | Send feedback Thanks for your co-operation. |
1 similar comment
Hi There, I am the AutoRest Linter Azure bot. I am here to help. My task is to analyze the situation from the AutoRest linter perspective. Please review the below analysis result: File: AutoRest Linter Guidelines | AutoRest Linter Issues | Send feedback Thanks for your co-operation. |
I'd like to remind you @zhanyu2014 that our License CLA bot still says that you are not appropriately documented as a Microsoft Employee. I cannot merge this PR until your CLA is recognized. |
Please acknowledge, @zhanyu2014. If I see no response, I'll have to close this PR as inactive. |
The related SLA has been validated and passed, related comments in change has been resolved, please help continue to finish this review and merge it. @marstr, @jianghaolu. |
The only thing remaining now is @ravbhatnagar or another ARM representative to sign off. Thanks for signing the CLA, @zhanyu2014. |
Ready for me to merge, @zhanyu2014? |
@marstr, please help us finish the code merge. |
…atabasename visible to customers
This checklist is used to make sure that common issues in a pull request are addressed. This will expedite the process of getting your pull request merged and avoid extra work on your part to fix issues discovered during the review process.
PR information
api-version
in the path should match theapi-version
in the spec).Quality of Swagger