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

Swagger updates to address docs and sdk issues #2139

Merged
merged 18 commits into from
Dec 19, 2017
Merged

Swagger updates to address docs and sdk issues #2139

merged 18 commits into from
Dec 19, 2017

Conversation

finiteattractor
Copy link
Contributor

@finiteattractor finiteattractor commented Dec 12, 2017

Found few issues while generating docs and SDK. Addressed by these changes. All test pass and SDK generation and test cases passing as well. Changes:

  • Fixed wrong enum definitions
  • IncludedUpdateClassifications to use comma separated strings
  • Fixed parameters not showing in docs
  • removed discriminator property, was resulting in missing property in generated models
  • First letter capital for better looking doc
  • Cleaned common definition file
  • fixed x-ms-enum without enum on same level error in code generation
  • removed subscriptionid/resourcegroupname/azureautomationname parameters to simplify sdk (they are not in the client)
  • Changed scheduleinfo to be a required property
  • moved filter parameter for better sdk parameter ordering
  • ran auto indent on files

eshaparmar and others added 17 commits September 11, 2017 12:42
Added extension properties to DSCNode for Get/GetNode api calls
Bring in the changes from Upstream to
AzureAutomationTeam
Adding new API version (2017-05-15-preview). Only three new resources added and all others reference the previous version. 

- Added resources:
 - SoftwareUpdateConfiguration
 - SoftwareUpdateConfigurationRun
 - SoftwareUpdateConfigurationMachineRun

- Validations:
 - Autorest azure validations passed
  - x-ms-examples validated with no errors
Updated tag in global configuration to use the latest api version as per azure PR review comment
- Moved parameters inside operations as it was not showing properly in documentation.
Fixing merge error
Fixing merge error
* Changed property type to integer

* includedUpdateClassifications to scalar integer

* model as string

* type to string

* Created adhoc property to allow code generation of enum with bitfield values since it seems that autorest doesn't support birfields and generate the enum only for string type property

* test

* Removed readonly attribute from name to allow setting outside constructor

* Fixed parameter type and replaced reference to parameter with actual definition

* removed descriminator property

* trying modelAsString with false

* modeled includedUpdateClassifications as string as deserialization was failing

* - First letter capital for better looking doc
- Replaces referece to paramaters with actual definitions on all resources
- Cleaned common definition file

* fixed x-ms-enum without enum on same level error in code generation

* removed subid/rg/aa parameters to simplify sdk

* added paemeters tat path level to avoid passing as arguments in sdk

* more fixes

* more fixes

* fixing more parameters

* Added missing automationAccountName parameter

* made resourceGroupName passed in function for account resource

* made schedule info requred property

* moved filter parameter for better sdk parameter ordering

* ran auto indent on files
@finiteattractor
Copy link
Contributor Author

Note: This is a resubmit of original PR (#2097). Been asked to resubmit from my own fork to address Travis issue with PRs originating from team forks.

@marstr marstr assigned marstr and unassigned dsgouda Dec 12, 2017
@marstr marstr added the WaitForARMFeedback <valid label in PR review process> add this label when ARM review is required label Dec 12, 2017
Copy link
Member

@marstr marstr left a comment

Choose a reason for hiding this comment

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

And just to confirm, this API Version is currently available, but in preview?

"swagger": "2.0",
"info": {
"title": "AutomationManagement",
"version": "2015-10-31"
Copy link
Member

Choose a reason for hiding this comment

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

Sorry I didn't notice this earlier, I'm a bit confused by the difference in API Version between the path to this file and how its declared here. Is this done to ship multiple API Versions together? If so, we can achieve the same thing more concisely by using tags that are found here: README.md@2165c04

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nope, you caught a copy/paste mistake :) this should be matching the version of the folder. I fixed it. Thanks!!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Correct, API is live and being used by Ibiza portal.

@dsgouda
Copy link
Contributor

dsgouda commented Dec 12, 2017

@marstr do you think you can review this since you have some context

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

AutoRest Linter Guidelines | AutoRest Linter Issues | Send feedback

Thanks for your co-operation.

@ravbhatnagar
Copy link
Contributor

@marstr - Only thing that popped from ARM point of view was that this has some breaking changes like making a property as required and enum changes. This if done in a new api-version is fine. Please confirm and looks good to merge other than that.

@finiteattractor
Copy link
Contributor Author

@ravbhatnagar this is a new api version and it is required property so we should be ok here.

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

AutoRest Linter Guidelines | AutoRest Linter Issues | Send feedback

Thanks for your co-operation.

@marstr marstr added potential-sdk-breaking-change and removed WaitForARMFeedback <valid label in PR review process> add this label when ARM review is required labels Dec 19, 2017
@marstr
Copy link
Member

marstr commented Dec 19, 2017

Adding the potential breaking changes label, but because this is in a new API Version, that's not a blocker.

@marstr marstr merged commit ca571e3 into Azure:current Dec 19, 2017
@AutorestCI
Copy link

No modification for AutorestCI/azure-sdk-for-python

@AutorestCI
Copy link

@AutorestCI
Copy link

No modification for AutorestCI/azure-sdk-for-python

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

Successfully merging this pull request may close these issues.

8 participants