Updating PUT operation requirements for SourceControls and SourceControlSyncJobs, updating streams’ value property, and updating examples. by Francisco-Gamino · Pull Request #3580 · Azure/azure-rest-api-specs

Francisco-Gamino · 2018-08-05T22:41:40Z

The PR contains the following changes:

Updating source control security token requirements for PUT operation.
Fixing SourceControlSyncJobs examples by replacing property 'syncJobId' for 'sourceControlSyncJobId'.
Making commitId property required and allowing empty values.
Making stream's value property an object instead of a string, and updating examples.

PR information

The title of the PR is clear and informative.
There are a small number of commits, each of which have an informative message. This means that previously merged commits do not appear in the history of the PR. For information on cleaning up the commits in your pull request, see this page.
Except for special cases involving multiple contributors, the PR is started from a fork of the main repository, not a branch.
If applicable, the PR references the bug/issue that it fixes.
Swagger files are correctly named (e.g. the api-version in the path should match the api-version in the spec).

Quality of Swagger

I have read the contribution guidelines.
My spec meets the review criteria:
- The spec conforms to the Swagger 2.0 specification.
- The spec follows the guidelines described in the Swagger checklist document.
- Validation tools were run on swagger spec(s) and have all been fixed in this PR.

…alues. Updating commitId description.

…g stream's examples.

AutorestCI · 2018-08-05T22:42:45Z

Automation for azure-sdk-for-python

The initial PR has been merged into your service PR:
Azure/azure-sdk-for-python#1959

AutorestCI · 2018-08-05T22:42:46Z

Automation for azure-sdk-for-java

Nothing to generate for azure-sdk-for-java

AutorestCI · 2018-08-05T22:43:30Z

Automation for azure-sdk-for-ruby

Nothing to generate for azure-sdk-for-ruby

azuresdkci · 2018-08-05T22:45:07Z

Can one of the admins verify this patch?

AutorestCI · 2018-08-05T22:46:26Z

Automation for azure-sdk-for-go

The initial PR has been merged into your service PR:
Azure/azure-sdk-for-go#2398

AutorestCI · 2018-08-05T22:46:47Z

Automation for azure-sdk-for-node

The initial PR has been merged into your service PR:
Azure/azure-sdk-for-node#3290

annatisch

Looks good - just a new minor things.
Just to confirm - are you aware that these changes are breaking and will result in a major version bump for all generated SDKs?

annatisch · 2018-08-05T22:56:16Z

...tomation/resource-manager/Microsoft.Automation/preview/2017-05-15-preview/sourceControl.json

@@ -493,8 +494,8 @@
        },
        "securityToken": {
          "type": "string",


Now that this is a complex object - please remove the "type":"string.

I missed that, thanks. Fixed.

annatisch · 2018-08-05T22:57:00Z

...tomation/resource-manager/Microsoft.Automation/preview/2017-05-15-preview/sourceControl.json

@@ -352,7 +352,8 @@
        },
        "securityToken": {
          "type": "string",


Now that this is a complex object - please remove the "type":"string.

annatisch · 2018-08-05T22:59:09Z

...tomation/resource-manager/Microsoft.Automation/preview/2017-05-15-preview/sourceControl.json

        "securityToken": {
          "type": "string",
-          "description": "Gets or sets the authorization token for the repo of the source control."
+          "description": "Gets or sets the authorization token for the repo of the source control.",


Please avoid terminology like "Gets or sets" as this isn't idiomatic for all generated SDK languages. Just stick with the description: "The authorization token for the repo of the source control."

Thank you for the suggestion. Fixed.

annatisch · 2018-08-05T23:00:15Z

...tomation/resource-manager/Microsoft.Automation/preview/2017-05-15-preview/sourceControl.json

          "type": "string",
-          "maxLength": 1024,
-          "description": "Gets or sets the authorization token for the repo of the source control."
+          "description": "Gets or sets the authorization token for the repo of the source control.",


Please avoid terminology like "Gets or sets" as this isn't idiomatic for all generated SDK languages. Just stick with the description. This also applies to the other descriptions below.

Francisco-Gamino · 2018-08-05T23:38:27Z

@annatisch: Thank you for the fast review. I've updated the specs, could you please take another look?

Regarding your comment: "Just to confirm - are you aware that these changes are breaking and will result in a major version bump for all generated SDKs?"
@vrdmr: Do you seen any issues with a major version bump for all regenerated Azure Automation SDKs?

Thanks!
Francisco

annatisch · 2018-08-05T23:50:52Z

Thanks @Francisco-Gamino for the quick response!

Whether you publish breaking changes to the SDKs is up to you - we generally recommend that service teams try to group as many breaking changes together so as to avoid frequently breaking releases - however you best know your customers and the frequency of major changes to your API :)

Once the CI passes I will take another look!

annatisch

Looks good to me! And thanks for fixing up the other descriptions :)

annatisch · 2018-08-06T00:18:54Z

Thanks @Francisco-Gamino - do you want me to merge now or are you waiting on any internal review?

Francisco-Gamino · 2018-08-06T00:39:21Z

Awesome! Thanks @annatisch! If you could merge it now, that would be great. Thanks again!

Francisco-Gamino added 4 commits August 5, 2018 15:27

Updating security token requirements for registering a source control.

2f79128

Replacing all instance of 'syncJobId' for 'sourceControlSyncJobId'.

9b116f2

Making commitId property required. Updating commitId to allow empty v…

0393994

…alues. Updating commitId description.

Making stream's value property an object instead of a string. Updatin…

ea69aa8

…g stream's examples.

AutorestCI mentioned this pull request Aug 5, 2018

[AutoPR automation/resource-manager] Updating PUT operation requirements for SourceControls and SourceControlSyncJobs, updating streams’ value property, and updating examples. Azure/azure-sdk-for-python#3078

Merged

azuresdkci assigned annatisch Aug 5, 2018

azuresdkci requested a review from annatisch August 5, 2018 22:42

AutorestCI mentioned this pull request Aug 5, 2018

[AutoPR automation/resource-manager] Updating PUT operation requirements for SourceControls and SourceControlSyncJobs, updating streams’ value property, and updating examples. Azure/azure-sdk-for-go#2396

Merged

AutorestCI mentioned this pull request Aug 5, 2018

[AutoPR automation/resource-manager] Updating PUT operation requirements for SourceControls and SourceControlSyncJobs, updating streams’ value property, and updating examples. Azure/azure-sdk-for-node#3289

Merged

annatisch reviewed Aug 5, 2018

View reviewed changes

Addressing PR comments.

2afd6df

annatisch approved these changes Aug 6, 2018

View reviewed changes

annatisch merged commit b310c2c into Azure:master Aug 6, 2018

Francisco-Gamino mentioned this pull request Aug 7, 2018

New SDK update for Automation - 3.3.0-preview Azure/azure-sdk-for-net#4641

Merged

9 tasks

Comments

Conversation

Francisco-Gamino commented Aug 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR information

Quality of Swagger

Uh oh!

AutorestCI commented Aug 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Automation for azure-sdk-for-python

Uh oh!

AutorestCI commented Aug 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Automation for azure-sdk-for-java

Uh oh!

AutorestCI commented Aug 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Automation for azure-sdk-for-ruby

Uh oh!

azuresdkci commented Aug 5, 2018

Uh oh!

AutorestCI commented Aug 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Automation for azure-sdk-for-go

Uh oh!

AutorestCI commented Aug 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Automation for azure-sdk-for-node

Uh oh!

annatisch left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Francisco-Gamino commented Aug 5, 2018

Uh oh!

annatisch commented Aug 5, 2018

Uh oh!

annatisch left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

annatisch commented Aug 6, 2018

Uh oh!

Francisco-Gamino commented Aug 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Francisco-Gamino commented Aug 5, 2018 •

edited

Loading

AutorestCI commented Aug 5, 2018 •

edited

Loading

AutorestCI commented Aug 5, 2018 •

edited

Loading

AutorestCI commented Aug 5, 2018 •

edited

Loading

AutorestCI commented Aug 5, 2018 •

edited

Loading

AutorestCI commented Aug 5, 2018 •

edited

Loading

annatisch left a comment •

edited

Loading

Francisco-Gamino commented Aug 6, 2018 •

edited

Loading