Skip to content
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

Update jobs.json to latest generated swagger file as well as it's updated examples #2909

Merged
merged 4 commits into from
May 17, 2018
Merged

Update jobs.json to latest generated swagger file as well as it's updated examples #2909

merged 4 commits into from
May 17, 2018

Conversation

johnpaulkee
Copy link
Contributor

@johnpaulkee johnpaulkee commented Apr 19, 2018

Summary

Updated sql readme.md to include jobs.json where necessary (will need confirmation if this is correctly placed)

Regenerated swagger to include job step and target executions, sku name and capacity support in agent createOrUpdate and get, as well as filter for top executions.

Job examples were updated to reflect correct json request and response paramters for agent and target group.

Review checklist

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

  • 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 ran this: oav validate-spec jobs.json --azure-validator and it gave no errors were found )

@AutorestCI
Copy link

AutorestCI commented Apr 19, 2018

Automation for azure-sdk-for-python

A 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:
Azure/azure-sdk-for-python#2285

@azuresdkciprbot
Copy link

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:

💡 Please review potentially introduced Error(s)/Warning(s): Analysis Report 💡

File: specification/sql/resource-manager/readme.md
Before the PR: Warning(s): 26 Error(s): 0
After the PR: Warning(s): 29 Error(s): 0

AutoRest Linter Guidelines | AutoRest Linter Issues | Send feedback

Thanks for your co-operation.

@AutorestCI
Copy link

AutorestCI commented Apr 19, 2018

Automation for azure-libraries-for-java

The initial PR has been merged into your service PR:
AutorestCI/azure-libraries-for-java#9

@AutorestCI
Copy link

AutorestCI commented Apr 19, 2018

Automation for azure-sdk-for-go

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

@AutorestCI
Copy link

AutorestCI commented Apr 19, 2018

Automation for azure-sdk-for-node

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

@hovsepm
Copy link
Contributor

hovsepm commented Apr 19, 2018

  "id": "1026",
  "code": "TypeChanged",
  "message": "The new version has a different type 'integer' than the previous one 'string'.",
  "jsonref": "#/paths/~1subscriptions~1{subscriptionId}~1resourceGroups~1{resourceGroupName}~1providers~1Microsoft.Sql~1servers~1{serverName}~1jobAgents~1{jobAgentName}~1executions/get/$skip",
  "json-path": "#/paths/subscriptions/{subscriptionId}/resourceGroups/{resourceGroupName}/providers/Microsoft.Sql/servers/{serverName}/jobAgents/{jobAgentName}/executions/get/$skip",
  "type": "Error"

You are introducing breaking changes to all the generated client SDKs. Should this be combined with API version bump PR?

@johnpaulkee
Copy link
Contributor Author

@hovsepm I think it's fine to keep this breaking change, but I think before we merge this PR I'll wait for @jaredmoo to sign off on this too since he's almost back from vacation.

@jaredmoo
Copy link
Contributor

This API is not yet published in the SDK, so there is technically no breaking change here.

Copy link
Contributor

@jaredmoo jaredmoo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great, just undo readme.md plz

@@ -75,6 +75,7 @@ input-file:
- Microsoft.Sql/preview/2017-03-01-preview/serverAutomaticTuning.json
- Microsoft.Sql/preview/2017-03-01-preview/serverDnsAliases.json
- Microsoft.Sql/preview/2017-03-01-preview/restorePoints.json
- Microsoft.Sql/preview/2017-03-01-preview/jobs.json
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please don't add to readme.md until we are ready to publish SDK.

@johnpaulkee
Copy link
Contributor Author

Undid readme.md, updating title to remove do not merge.

@johnpaulkee johnpaulkee changed the title [DO NOT MERGE] Update jobs.json to latest generated swagger file as well as it's updated examples Update jobs.json to latest generated swagger file as well as it's updated examples Apr 20, 2018
Copy link
Contributor

@jaredmoo jaredmoo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@azuresdkciprbot
Copy link

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: specification/sql/resource-manager/readme.md
Before the PR: Warning(s): 26 Error(s): 0
After the PR: Warning(s): 26 Error(s): 0

AutoRest Linter Guidelines | AutoRest Linter Issues | Send feedback

Thanks for your co-operation.

@hovsepm
Copy link
Contributor

hovsepm commented Apr 24, 2018

@johnpaulkee we will need ARM to sign off on the entire swagger spec. When are you planning to undergo API review?

@hovsepm hovsepm added the DoNotMerge <valid label in PR review process> use to hold merge after approval label Apr 24, 2018
@jaredmoo
Copy link
Contributor

It has been reviewed internally by email and already lit up, but I don't think @ravbhatnagar has explicitly approved on github.

@hovsepm hovsepm added the WaitForARMFeedback <valid label in PR review process> add this label when ARM review is required label Apr 24, 2018
@hovsepm
Copy link
Contributor

hovsepm commented Apr 24, 2018

@ravbhatnagar could you take a look to the entire swagger? We never got a sign-off from ARM.

}
}
},
"/subscriptions/{subscriptionId}/resourceGroups/{resourceGroupName}/providers/Microsoft.Sql/servers/{serverName}/jobAgents/{jobAgentName}/jobs/{jobName}/executions/{jobExecutionId}/steps/{stepName}": {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

GET on /targets or /steps returns the same entity - JobExecution?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Returns JobStepExecutions for /steps and JobTargetExecutions for /targets.
Returned model is similar, except TargetExecutions includes StepName and TargetId which helps identify which target this step (script) was run on.

@ravbhatnagar
Copy link
Contributor

Signing off!

@ravbhatnagar ravbhatnagar added ARMSignedOff <valid label in PR review process>add this label when ARM approve updates after review and removed WaitForARMFeedback <valid label in PR review process> add this label when ARM review is required labels Apr 26, 2018
@hovsepm
Copy link
Contributor

hovsepm commented Apr 26, 2018

@johnpaulkee @jaredmoo Please add the readme.md changes back and keep this PR open until your service will be deployed and SDK's could be generated.

@hovsepm
Copy link
Contributor

hovsepm commented Apr 26, 2018

Also please fix linter error - https://travis-ci.org/Azure/azure-rest-api-specs/jobs/369280805

@hovsepm
Copy link
Contributor

hovsepm commented Apr 26, 2018

Please fix sample issues reported in sample validation - https://travis-ci.org/Azure/azure-rest-api-specs/jobs/369280815

@jaredmoo
Copy link
Contributor

The linter error is expected. Operation API is defined in a different file (operations.json)

The Missing required property: password model errors are all bugs in validator. "Password" is a required write-only property and it is intentionally null in responses.

The tablename errors (e.g. https://travis-ci.org/Azure/azure-rest-api-specs/jobs/369280815#L900 ) might be legit. @johnpaulkee I'll let you check it out.

@jaredmoo
Copy link
Contributor

Filed Azure/oav#239 for the OAV false positive.

@johnpaulkee johnpaulkee changed the title Update jobs.json to latest generated swagger file as well as it's updated examples [Do not merge - Waiting for public preview] Update jobs.json to latest generated swagger file as well as it's updated examples Apr 27, 2018
@johnpaulkee
Copy link
Contributor Author

I believe I fixed the job step output example issues. Waiting on the build validation to confirm.
Also updated readme.md to include jobs.json.
@jaredmoo can you confirm if readme.md is correct?

@johnpaulkee johnpaulkee changed the title [Do not merge - Waiting for public preview] Update jobs.json to latest generated swagger file as well as it's updated examples Update jobs.json to latest generated swagger file as well as it's updated examples May 17, 2018
@johnpaulkee
Copy link
Contributor Author

I've also removed [Do not merge] from the title as we're intending on checking this in since we're getting close to public preview and we've received ARM sign off.

Copy link
Contributor

@jaredmoo jaredmoo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor comments. Thanks!

@@ -51,6 +51,8 @@ Differences in v3 (compared to v2):
- `+2017-10-01-preview/capabilities.json`
- `+2017-10-01-preview/databases.json`
- `+2017-10-01-preview/elasticPools.json`
- New SQL database agent APIs
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should not be a difference between v3 and v2, because jobs.json is in all composite version (v3, v2, v1)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In other words: please delete the new line 54 & 55 :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You got it :)

@@ -287,6 +292,7 @@ input-file:
- Microsoft.Sql/preview/2017-03-01-preview/serverAutomaticTuning.json
- Microsoft.Sql/preview/2017-03-01-preview/serverDnsAliases.json
- Microsoft.Sql/preview/2017-03-01-preview/restorePoints.json
- Microsoft.Sql/preview/2017-03-01-preview/jobs.json
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please keep in alphabetical order (in all the sections you added to)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will update to be in alphabetical order.

…in it's 2017-03-01-preview regions as well as removed comment for difference between v3 and v2
@johnpaulkee
Copy link
Contributor Author

Updated based on feedback from @jaredmoo.
@hovsepm when you get a chance can you review once more?
We're ready to publish this :)

@hovsepm hovsepm removed the DoNotMerge <valid label in PR review process> use to hold merge after approval label May 17, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ARMSignedOff <valid label in PR review process>add this label when ARM approve updates after review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants