-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
[AutoPR] policyinsights/resource-manager #2122
Conversation
…hange modelAsString for x-ms-enum to true (#2107) * Generated from 648b3651cc044b52ce58431a57ba3b7f7392fc2d Reorganize files; change modelAsString for x-ms-enum to true Reorganize files (remove resource type based subfolders; rename conflicting example files to have resource type name; change swagger spec files and MD file based on new file locations and names); change modelAsString for x-ms-enum to true based on API review board recommendation * Generated from 14bce7a312971c5831a8e2bd1f70d167bd42d96d Dummy change to kick the CI again
Codecov Report
@@ Coverage Diff @@
## master #2122 +/- ##
==========================================
+ Coverage 54.37% 55.16% +0.79%
==========================================
Files 5979 6450 +471
Lines 135524 146041 +10517
==========================================
+ Hits 73686 80560 +6874
- Misses 61838 65481 +3643
Continue to review full report at Codecov.
|
@lmazuel For the first PR we had in swagger repo (Azure/azure-rest-api-specs#2523), an automatic PR was created here #2026. At the time there was an issue with folders and so we had decided to close it. swagger repo PR was merged with changes but looks like no new PR was created here at the time. Now we are releasing swagger specs for our new non-preview release as part of Azure/azure-rest-api-specs#2929 which looks like triggered this PR. Two issues I'm trying to wrap my head around: 1. I see that the files generated are for our preview specs. What will happen now? 2. Build CI seems to have failed though Details link take me to 404. What is the issue? Could you help? I'm looking at releasing Phyton SDK for our RP, either for our preview API version, or ideally for the new API version. |
(message created by the CI based on PR content) Installation instructionPackage azure-mgmt-policyinsightsYou can install the package You can build a wheel to distribute for test using the following command: If you have a local clone of this repository, you can also do:
Or build a wheel file to distribute for testing:
Direct downloadYour files can be directly downloaded here: |
|
…cyinsights/resource-manager
@lmazuel Thank you! So this PR (which reflects the state of things in specs master) will be merged when I test the package and make sure it is OK. Is there any other steps required? And 2452 will be merged when 2929 is merged? |
Correct, 2452 will be merged automatically when 2929 will be merged. If you just want to test the GA version, you can add test to PR 2452, they will eventually come here. |
Merge remote-tracking branch 'upstream/master'
@lmazuel As soon as I'm done testing this, I will let you know (by Monday I think). Is there anything else needed that I should do before this PR is approved and merged? When merged, I assume customers can install it from this GitHub? When would it go online in PyPI and when would it be part of azure package? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bulentelmaci request changes to be sure you see my comment.
|
||
def __init__(self, **kwargs): | ||
super(PolicyEventsQueryResults, self).__init__(**kwargs) | ||
self.odatacontext = kwargs.get('odatacontext', None) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bulentelmaci Do you need odatacontext
and odatacount
here? If not, this API should be declared as producing a list (that's the case, you produce a list of PolicyEvent). This makes better SDK, and it's just adding a x-ms-pageable
extension in the Swagger.
Don't be fooled by the name of the extension, this does NOT require the call to be pageable. This is an odd choose to say "the result is a list". Pageable a sub-set of "result is a list".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lmazuel Swagger folks suggested the use of x-ms-pageable as well but then we decided not to since we don't support (or no near plans to support) paging. The generated API still produces list. From what I understand from https://github.com/Azure/autorest/blob/master/docs/extensions/readme.md#x-ms-pageable, this was specifically designed to model next links and have SDKs generated that follow the pattern. For us that is not needed.
Yes, we need odata count and context. OData client generated clients choke without them.
@bulentelmaci For your general questions, merge and PyPI are the same thing, I only merge when it's ready to be on PyPI. So customers will be able to install from Github or PyPI. |
@lmazuel I tested the generated Python package and did not see any issues. Can we merge/publish this then? |
Created to accumulate context: policyinsights/resource-manager