-
Notifications
You must be signed in to change notification settings - Fork 5.1k
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
Panwang/taskruns #7123
Panwang/taskruns #7123
Conversation
In Testing, Please Ignore[Logs] (Generated from d0215e3, Iteration 8).NET: test-repo-billy/azure-sdk-for-net [Logs]
Python: test-repo-billy/azure-sdk-for-python [Logs]
Java: test-repo-billy/azure-sdk-for-java [Logs]
Go: test-repo-billy/azure-sdk-for-go [Logs] [Diff]
JavaScript: test-repo-billy/azure-sdk-for-js [Logs]
Ruby: test-repo-billy/azure-sdk-for-ruby [Logs]
|
Automation for azure-sdk-for-pythonA PR has been created for you: |
Automation for azure-sdk-for-goThe initial PR has been merged into your service PR: |
Can one of the admins verify this patch? |
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.
Looks good from SDK side. Requested ARM review as it adds new APIs.
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.
Please see comments
...-manager/Microsoft.ContainerRegistry/preview/2019-06-01-preview/containerregistry_build.json
Outdated
Show resolved
Hide resolved
...-manager/Microsoft.ContainerRegistry/preview/2019-06-01-preview/containerregistry_build.json
Outdated
Show resolved
Hide resolved
...-manager/Microsoft.ContainerRegistry/preview/2019-06-01-preview/containerregistry_build.json
Show resolved
Hide resolved
"type": "Microsoft.ContainerRegistry/registries/TaskRuns", | ||
"properties": { | ||
"provisioningState": "Creating", | ||
"runRequest": { |
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.
As described above, the swagger spec doesn't document this schema or the other types.
hi @pan-wang could you pls address comments ? |
Thank so much for remaindering.
We are working on twitting the change for better ARM deployment experience. Will address all feedbacks soon once we land on final change.
Thanks
Pan
…________________________________
From: Yunge Zhu <[email protected]>
Sent: Monday, October 7, 2019 11:32 PM
To: Azure/azure-rest-api-specs <[email protected]>
Cc: pan-wang <[email protected]>; Mention <[email protected]>
Subject: Re: [Azure/azure-rest-api-specs] Panwang/taskruns (#7123)
hi @pan-wang<https://github.com/pan-wang> could you pls address comments ?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub<#7123?email_source=notifications&email_token=AEKUVW7X75CINWR4D35NII3QNP5NJA5CNFSM4ITX26M2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEASQXII#issuecomment-539298721>, or mute the thread<https://github.com/notifications/unsubscribe-auth/AEKUVW3F7SCAWTDESRLVROTQNP5NJANCNFSM4ITX26MQ>.
|
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.
@KrisBash's comments have not been addressed and are blocking approval. Please fix.
@KrisBash Could we move forward for this PR? We would like to file a separate work item to solve the debt per discussion above. And there're two Ignore Test-Only CI fail. We can just ignore, right? |
...-manager/Microsoft.ContainerRegistry/preview/2019-06-01-preview/examples/TaskRunsCreate.json
Show resolved
Hide resolved
I think you meant @KrisBash. For the CI failures, @anuchandy will need to weigh as the SDK team owns these checks. |
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.
Looks good to me. I'll leave it for @KrisBash to do the final sign off since his comments are still remaining.
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.
@pan-wang - one comment, please take a look. ALso some from Kris have not been addressed.
@@ -2072,6 +2491,27 @@ | |||
}, | |||
"x-ms-azure-resource": true | |||
}, | |||
"Error": { |
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.
this is not the right schema for error. sample here - https://github.com/Azure/azure-rest-api-specs/blob/master/specification/batch/resource-manager/Microsoft.Batch/stable/2019-08-01/BatchManagement.json#L105
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.
@ravbhatnagar Just submitted a new commit to solve the problem
Signing off from ARM side. |
@majastrz ARM signed off this change. Could you continue merge the manifest? |
@anuchandy Could you merge the PR? The manifest update was blocked now. |
095677d
to
fca508d
Compare
216765f
to
f14a1fb
Compare
Latest improvements:
MSFT employees can try out our new experience at OpenAPI Hub - one location for using our validation tools and finding your workflow.
Contribution checklist:
ARM API Review Checklist
Failure to comply may result in delays for manifest application. Note this does not apply to data plane APIs.
Please follow the link to find more details on API review process.