-
Notifications
You must be signed in to change notification settings - Fork 5.6k
Initial Regional Move Service tsp [do not merge] #39076
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
base: main
Are you sure you want to change the base?
Conversation
Next Steps to MergeNext steps that must be taken to merge this PR:
Comment generated by summarize-checks workflow run. |
API Change CheckAPIView identified API level changes in this PR and created the following API reviews
|
ceb5772 to
ee89360
Compare
ee89360 to
d55336c
Compare
ef1ed4e to
efe2a52
Compare
efe2a52 to
98f45f9
Compare
|
the sdk validation is failing. Please check. |
|
@msyyc The Python SDK validation is failing with the error below. It seems unrelated to my changes: |
@psah434 These appear to be infra issues and I am following up with the owners. Please continue with the review as the fixes for those issues are unlikely to change the tsp shape. |
Python failure is caused by wrong configuration in tspconfig.yaml. I commit the fix to the PR which shall fix the failure. |
Thanks for fixing it! The Python validation is passing now. |
|
@psah434 The .net SDK failure is due to Azure/azure-sdk-for-net#54383. I've addressed the other .net validation failures, however. |
| from: regionalMove.json | ||
| reason: Operations API is implemented as a separate service. | ||
| - suppress: AvoidAdditionalProperties | ||
| from: regionalMove.json |
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.
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.
Since there's one Swagger file being generated, I assume you mean path within the file?
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.
Never mind. I found examples of how to do it.
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.
I narrowed the scope of the suppression to just the single definition.
|
|
||
| delete is ArmResourceDeleteSync<RegionalMoveContainer>; | ||
|
|
||
| @action("finalize") |
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.
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.
Will rename to finalizeMove.
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.
Renamed
| "resourceProperties": { | ||
| "type": "object", | ||
| "description": "Properties to be overridden/modified on the resource during the copy operation. The resource provider will reject invalid modifications.", | ||
| "additionalProperties": {} |
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.
Use of additionalProperties is not allowed for properties owned by the service. The only time its ok to use it is when the properties are pass thru (user defined) and not subject to any validations.
Please provide how these values are used.
If your team uses these values, Please make this an array or provide an explanation for why you need this.
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.
@ramoka178 The resourceProperties are passed through as-is to the RP that implements the internal move contract with ARM. We don't do anything with those values except pass them through to the RP.
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.
I renamed resourceProperties to resourceOverrides to clarify the intent. The shape is basically a subset of a full ARM resource body and can include properties like sku, zones, properties, or others depending on the RP. As I said earlier, we will pass the object as-is to the RP.
| "type": "string", | ||
| "description": "Resource ID where the source will be copied/moved to. For copy operations, this cannot be the same as the source." | ||
| }, | ||
| "resourceProperties": { |
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.
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.
Will do.
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.
Added
ARM (Control Plane) API Specification Update Pull Request
Tip
Overwhelmed by all this guidance? See the
Getting helpsection at the bottom of this PR description.PR review workflow diagram
Please understand this diagram before proceeding. It explains how to get your PR approved & merged.
Purpose of this PR
What's the purpose of this PR? Check the specific option that applies. This is mandatory!
Due diligence checklist
To merge this PR, you must go through the following checklist and confirm you understood
and followed the instructions by checking all the boxes:
ARM resource provider contract and
REST guidelines (estimated time: 4 hours).
I understand this is required before I can proceed to the diagram Step 2, "ARM API changes review", for this PR.
Additional information
Viewing API changes
For convenient view of the API changes made by this PR, refer to the URLs provided in the table
in the
Generated ApiViewcomment added to this PR. You can use ApiView to show API versions diff.Suppressing failures
If one or multiple validation error/warning suppression(s) is detected in your PR, please follow the
suppressions guide to get approval.
Getting help
Purpose of this PRandDue diligence checklist.write accessper aka.ms/azsdk/access#request-access-to-rest-api-or-sdk-repositoriesNext Steps to Mergecomment. It will appear within few minutes of submitting this PR and will continue to be up-to-date with current PR state.and https://aka.ms/ci-fix.
queuedstate, please add a comment with contents/azp run.This should result in a new comment denoting a
PR validation pipelinehas started and the checks should be updated after few minutes.