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

add Azure Container Registry policies api and update OperationDefinition #3094

Merged
merged 1 commit into from
Jul 3, 2018

Conversation

ankurkhemani
Copy link
Contributor

@ankurkhemani ankurkhemani commented May 17, 2018

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 May 17, 2018

Automation for azure-sdk-for-node

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-node#3111

@AutorestCI
Copy link

AutorestCI commented May 17, 2018

Automation for azure-libraries-for-java

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:
AutorestCI/azure-libraries-for-java#106

@AutorestCI
Copy link

AutorestCI commented May 17, 2018

Automation for azure-sdk-for-python

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

@AutorestCI
Copy link

AutorestCI commented May 17, 2018

Automation for azure-sdk-for-go

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

@ankurkhemani
Copy link
Contributor Author

@ankurkhemani ankurkhemani force-pushed the ankheman/acr-policies-swagger branch from 3f66115 to 93ffd75 Compare May 18, 2018 01:06
@ankurkhemani ankurkhemani changed the title add Azure Container Registry policies api and update OperationDefinition [Please do not review yet] add Azure Container Registry policies api and update OperationDefinition May 18, 2018
@ankurkhemani ankurkhemani force-pushed the ankheman/acr-policies-swagger branch 2 times, most recently from 560cb74 to a13586d Compare May 18, 2018 01:45
@ankurkhemani ankurkhemani changed the title [Please do not review yet] add Azure Container Registry policies api and update OperationDefinition add Azure Container Registry policies api and update OperationDefinition May 18, 2018
@ankurkhemani
Copy link
Contributor Author

ankurkhemani commented May 18, 2018

@marstr I don't understand why the CI complains when adding values to enum property. Can you please take a look?

INFO:swaggertosdk.autorest_tools:TypeError: metaclass conflict: the metaclass of a derived class must be a (non-strict) subclass of the metaclasses of all its bases

@marstr
Copy link
Member

marstr commented May 19, 2018

@lmazuel, is adding enum properties categorically breaking, or is this something to do with the way that value was added?

@marstr marstr added the WaitForARMFeedback <valid label in PR review process> add this label when ARM review is required label May 21, 2018
@marstr
Copy link
Member

marstr commented May 21, 2018

@ravbhatnagar may be able to help shed some light here too.

@ravbhatnagar
Copy link
Contributor

Yes adding values to an enum is a breaking change. The reason being a strongly typed SDK will not be able to deserialize a value it doesnt understand. To avoid this, enums need to be categorized as modelAsString:true.

@ankurkhemani
Copy link
Contributor Author

@ravbhatnagar We have set modelAsString: true for the enums.

@marstr
Copy link
Member

marstr commented May 23, 2018

Let me know when you're ready to merge

edit: @ravbhatnagar still has the WaitForARMFeedback label on this. We should still wait for his approval.

@ankurkhemani
Copy link
Contributor Author

@ravbhatnagar Can you please review the swagger changes?

@ravbhatnagar
Copy link
Contributor

Looks good!

@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 May 29, 2018
@marstr
Copy link
Member

marstr commented May 30, 2018

Ready for me to merge, @ankurkhemani ?

@ankurkhemani
Copy link
Contributor Author

@marstr Let's wait for the ARM manifest change to be applied .. I will let you know. Thanks!

@marstr marstr added the DoNotMerge <valid label in PR review process> use to hold merge after approval label May 30, 2018
@marstr
Copy link
Member

marstr commented Jun 19, 2018

Any updates here, or an estimated time-line for when these changes should be applied?

@marstr
Copy link
Member

marstr commented Jun 20, 2018

Closing as stale. Feel free to ping this thread to have it re-opened.

@AutorestCI
Copy link

AutorestCI commented Jun 21, 2018

Automation for azure-sdk-for-java

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-java#2176

@AutorestCI
Copy link

AutorestCI commented Jun 21, 2018

Automation for azure-sdk-for-ruby

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

@ankurkhemani
Copy link
Contributor Author

ankurkhemani commented Jun 29, 2018

@marstr Can you please re-open this PR and merge it?

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.

4 participants