Skip to content

Created specs for python 2 packages#3713

Merged
jhendrixMSFT merged 5 commits intoAzure:masterfrom
D1v38om83r:bhbrahma-pr
Sep 27, 2018
Merged

Created specs for python 2 packages#3713
jhendrixMSFT merged 5 commits intoAzure:masterfrom
D1v38om83r:bhbrahma-pr

Conversation

@D1v38om83r
Copy link
Contributor

Swagger spec update for new feature. Python 2 package support in Azure Automation.

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

@AutorestCI
Copy link

AutorestCI commented Aug 23, 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#3125

@D1v38om83r
Copy link
Contributor Author

@ravbhatnagar can you please review on behalf of the ARM team. I am introducing a new resource here that is practically the same as Azure automation Module (powershell), but for python.

@AutorestCI
Copy link

AutorestCI commented Aug 23, 2018

Automation for azure-sdk-for-java

Nothing to generate for azure-sdk-for-java

@AutorestCI
Copy link

AutorestCI commented Aug 23, 2018

Automation for azure-sdk-for-ruby

Nothing to generate for azure-sdk-for-ruby

@azuresdkci
Copy link
Contributor

Can one of the admins verify this patch?

@AutorestCI
Copy link

AutorestCI commented Aug 23, 2018

Automation for azure-sdk-for-node

A PR has been created for you:
Azure/azure-sdk-for-node#3414

@AutorestCI
Copy link

AutorestCI commented Aug 23, 2018

Automation for azure-sdk-for-go

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

@jhendrixMSFT jhendrixMSFT added the WaitForARMFeedback <valid label in PR review process> add this label when ARM review is required label Aug 27, 2018
@D1v38om83r
Copy link
Contributor Author

ping!
@ravbhatnagar can you please review on behalf of the ARM team. I am introducing a new resource here that is practically the same as Azure automation Module (powershell), but for python.

},
"ModuleProperties": {
"properties": {
"isGlobal": {
Copy link
Contributor

Choose a reason for hiding this comment

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

bools are typically not recommended as they are less descriptive and dont allow for future expansion of states if needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The property isGlobal is set in out data model as a boolean everywhere. Its not expected to change ever. The specs for Python 2 packages are practically the same a Powershell modules for consistency, except for paths.

"type": "string",
"description": "Gets or sets the description."
},
"isComposite": {
Copy link
Contributor

Choose a reason for hiding this comment

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

same comment for bool

Copy link
Contributor Author

Choose a reason for hiding this comment

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

isComposite is never expected to change. The specs for Python 2 packages are practically the same a Powershell modules for consistency, except for paths.

"x-ms-client-flatten": true,
"description": "Gets or sets the module update properties."
},
"name": {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is name of the resource really updatable?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no, only the tag is updatable. Left in there for consistency with PowerShell modules.

Copy link
Contributor

Choose a reason for hiding this comment

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

Consistency at the expense of something which doesnt work is not recommended. Please fix.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have removed the parameters which have no effect.

"type": "string",
"description": "Gets or sets name of the resource."
},
"location": {
Copy link
Contributor

Choose a reason for hiding this comment

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

location of azure resources is not updatbale.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, that is correct. Left in there for consistency with PowerShell modules.

Copy link
Contributor

Choose a reason for hiding this comment

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

same comment as above. lets fix this. And even fix powershell in a new api-version.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have removed the parameters which have no effect.

"x-ms-client-flatten": true,
"description": "Gets or sets the module create properties."
},
"name": {
Copy link
Contributor

Choose a reason for hiding this comment

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

name comes from the URL. It should be readonly in the request body

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Parameter name has been dropped

],
"description": "The parameters supplied to the create or update module properties."
},
"ModuleCreateOrUpdateParameters": {
Copy link
Contributor

Choose a reason for hiding this comment

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

why is the same model no used for request and response? It helps in GET-PUT pipeline and makes it simpler to consume

@jhendrixMSFT
Copy link
Member

Please be sure to review the fixes made to the powershell equivalent as there were some bugs in that swagger which made the "GetContent" API not work, see the history.

@jhendrixMSFT
Copy link
Member

@D1v38om83r any update on ARM feedback?

@jhendrixMSFT
Copy link
Member

@D1v38om83r there is a merge conflict, can you please rebase your changes on top of master?

@jhendrixMSFT
Copy link
Member

@D1v38om83r ping

@AutorestCI
Copy link

AutorestCI commented Sep 20, 2018

Automation for azure-sdk-for-js

Nothing to generate for azure-sdk-for-js

@D1v38om83r
Copy link
Contributor Author

@jhendrixMSFT the merge conflicts have been resolved

@jhendrixMSFT
Copy link
Member

@D1v38om83r thanks! Where are we with the ARM review?

@jhendrixMSFT
Copy link
Member

@D1v38om83r also has this service been deployed?

@D1v38om83r
Copy link
Contributor Author

D1v38om83r commented Sep 24, 2018

@ravbhatnagar can you please take a look at the changes I made addressing your PR comments.
@jhendrixMSFT yes the service has been deployed

@jhendrixMSFT
Copy link
Member

Looking at the ARM feedback I think the only remaining issue is why is the same model not used for request and response. Can this be fixed?

@D1v38om83r
Copy link
Contributor Author

The request and response are too dissimilar, there is next to no overlap. It doesn't make sense to combine them.

@jhendrixMSFT
Copy link
Member

Thanks for the explanation this makes sense. Will merge EOD unless @ravbhatnagar has any objections.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

WaitForARMFeedback <valid label in PR review process> add this label when ARM review is required

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants