Azure container registry build feature GA API specification and examples. #3617

mnltejaswini · 2018-08-09T18:23:20Z

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

I have read the contribution guidelines.
My spec meets the review criteria:
- The spec conforms to the Swagger 2.0 specification.
- The spec follows the guidelines described in the Swagger checklist document.
- Validation tools were run on swagger spec(s) and have all been fixed in this PR.

AutorestCI · 2018-08-09T18:24:12Z

Automation for azure-sdk-for-python

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-python#3140

AutorestCI · 2018-08-09T18:24:36Z

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#3337

AutorestCI · 2018-08-09T18:24:45Z

Automation for azure-sdk-for-ruby

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

azuresdkci · 2018-08-09T18:25:09Z

Can one of the admins verify this patch?

AutorestCI · 2018-08-09T18:25:48Z

Automation for azure-sdk-for-go

Nothing to generate for azure-sdk-for-go

AutorestCI · 2018-08-09T18:26:58Z

Automation for azure-sdk-for-java

Nothing to generate for azure-sdk-for-java

annatisch · 2018-08-09T18:29:30Z

Adding @ravbhatnagar to review new ARM API version swagger.

@mnltejaswini - there are some model validation errors in the examples - could you take a look?https://travis-ci.org/Azure/azure-rest-api-specs/jobs/414189551

Thanks!

annatisch · 2018-08-10T22:15:19Z

Thanks for the updates @mnltejaswini
Could you please update the readme tag so that the generated SDKs pick up the latest API version?
https://github.com/Azure/azure-rest-api-specs/pull/3617/files#diff-4837e14bb0f77202f522d1f223f87c3aR29

Unless you have a particular reason not to publish SDKs for this API?

Finally, there's still a couple of errors in the Tasks_Get example:

 error: 
operationId: Tasks_Get
scenario: Tasks_Get
source: response
responseCode: '200'
severity: 0
errorCode: OBJECT_MISSING_REQUIRED_PROPERTY
errorDetails:
  code: OBJECT_MISSING_REQUIRED_PROPERTY
  params:
    - name
  message: 'Missing required property: name'
  path: properties/trigger/sourceTriggers/0
  title: SourceTrigger
  description: The properties of a source based trigger.

 error: 
operationId: Tasks_Get
scenario: Tasks_Get
source: response
responseCode: '200'
severity: 0
errorCode: OBJECT_MISSING_REQUIRED_PROPERTY
errorDetails:
  code: OBJECT_MISSING_REQUIRED_PROPERTY
  params:
    - name
  message: 'Missing required property: name'
  path: properties/trigger/baseImageTrigger
  title: BaseImageTrigger
  description: The trigger based on base image dependency.

johanste · 2018-08-14T21:39:49Z

.../resource-manager/Microsoft.ContainerRegistry/stable/2018-09-01/containerregistry_build.json

+        }
+      }
+    },
+    "/subscriptions/{subscriptionId}/resourceGroups/{resourceGroupName}/providers/Microsoft.ContainerRegistry/registries/{registryName}/getBuildSourceUploadUrl": {


Is this API ever expected to be consumed from an ARM template? If so, @ravbhatnagar, is it still true that the name should be list*** (regardless of how many items are actually returned)?

Yes, you are correct. It cannot be called from ARM template if it is not prefixed with list. There is no use case for this API to be called from template

In general, I've often found that there are scenarios for consuming pretty much any computed value from within a template. So, my default recommendation is to take an "assume it will be consumed from within a template unless proven otherwise" approach. But this is a general statement. If you truly don't have any scenarios where one would, say, provision a virtual machine that needs the access the value (or that user assigned identities would enable the access from the resource itself), then this is all good.

I renamed it to prefix with list so that it can be invoked from template.

johanste · 2018-08-14T21:41:48Z

.../resource-manager/Microsoft.ContainerRegistry/stable/2018-09-01/containerregistry_build.json

+          {
+            "name": "$filter",
+            "in": "query",
+            "description": "The runs filter to apply on the operation.",


What capabilities does the filter have? Full odata (operators and all properties)? If not, specifying what the user can put in the $filter in the description is very helpful for callers...

Makes sense. there are some restrictions on the odata query , I will add them to the description.

I updated the description to include restrictions on filter.

johanste · 2018-08-14T21:52:47Z

.../resource-manager/Microsoft.ContainerRegistry/stable/2018-09-01/containerregistry_build.json

+            "format": "int32"
+          },
+          {
+            "name": "$skipToken",


How would the user get the skip token value? If the only way to get it is from a nextLink in the response, then there is limited value in documenting the parameter, since the only way that the user can get it is by parsing the URL, and then putting it back into the API to build up exactly the same URL as they got in the nextLink to begin with....

The nextLink is self contained and can be invoked as is. Please correct me if my understanding is wrong?

Is there some other way that I, as a user, can determine what value I could possibly pass in for the $skipToken parameter?

No, the only way is from nextLink

@mnltejaswini - I think what @johanste is saying is that you can probably remove this parameter because the generated SDKs already support retrieving the next page directly from nextLink - so unless there's a particular situation where a customer would need to extract the skipToken from nextLink, it's not really necessary.

mnltejaswini · 2018-08-14T21:54:45Z

@johanste Is it allowed for the PATCH and PUT request payload is of same type? The lint checker complaints about the conflicts between required and patchable properties. So we either remove Required for Task and do only server validation versus separate the create and update parameters. Which one is preferred? Can you please advise

johanste · 2018-08-14T21:59:28Z

@mnltejaswini, having required parameters for a PATCH request body rarely makes sense. You want to provide only the value that you want to modify, which is orthogonal to if they are required on create/PUT.

The most common pattern is to have separate models. Unfortunately, there is no way in swagger/openapi to have selectively required parameters depending on the point of usage.

mnltejaswini · 2018-08-14T22:07:25Z

@johanste Thank you. Will separate the properties.

ravbhatnagar · 2018-08-15T07:34:09Z

pending a couple of updates from @mnltejaswini. Please let me know once you are ready and I can review.

…ers to avoid conflict between required nd patchable properties.

mnltejaswini · 2018-08-15T22:12:00Z

@ravbhatnagar I separated the create and update parameters to avoid conflict between required and patchable properties. Can you please review.

mnltejaswini · 2018-08-20T17:25:51Z

@ravbhatnagar Can you please review

ravbhatnagar · 2018-08-22T09:27:20Z

This was reviewed in person. Signing off!

annatisch · 2018-08-22T17:45:01Z

Thanks @ravbhatnagar!
@mnltejaswini - could you please take a look at the validation errors in the examples?

…rguments property to values for Task type requests and steps

mnltejaswini · 2018-08-23T18:57:50Z

@ravbhatnagar As per the review board feedback, there are minor changes, added isArchiveEnabled as part of schedule run request and renamed the arguments to values property for task type step and run requests.

mnltejaswini · 2018-08-23T19:01:23Z

@annatisch where can I find the validation errors in examples? Never mind, I found them. will update the examples.

annatisch · 2018-08-23T19:02:32Z

Thanks @mnltejaswini - you can find them here:
https://travis-ci.org/Azure/azure-rest-api-specs/jobs/419797791

I will also review the PR today.

mnltejaswini · 2018-08-23T19:04:40Z

@annatisch They doesn't seem to be valid. The payload for GET and PUT is the same, while we can PUT some sensitive information as part of resource creation, we cannot return them as part of GET. This has be the pattern we were using for all our resources.

… to fix the model validation failures.

annatisch

Just a few small things :)

annatisch · 2018-08-23T22:01:15Z

.../resource-manager/Microsoft.ContainerRegistry/stable/2018-09-01/containerregistry_build.json

+          "Registries"
+        ],
+        "description": "Get the upload location for the user to be able to upload the source.",
+        "operationId": "Registries_ListBuildSourceUploadUrl",


I see this operation has been changed from "GetBuildSourceUploadUrl" to "ListBuildSourceUploadUrl".
Given that it doesn't actually list anything - could we please change it back to "GetBuildSourceUploadUrl"?

same here, in the swagger review, it is recommended to prefix it with list so that they can be used in ARM template.

Thanks @mnltejaswini - the operation ID is only for the generated clients and does not impact the URL or ARM templates.

annatisch · 2018-08-23T22:14:03Z

.../resource-manager/Microsoft.ContainerRegistry/stable/2018-09-01/containerregistry_build.json

+            "description": "The run update properties.",
+            "required": true,
+            "schema": {
+              "$ref": "#/definitions/RunUpdateParameters"


Does this mean that a customer can only update the Run "isArchiveEnabled"? And not any of the subclass specific fields (e.g. DockerBuildRequest.image_names etc etc)

No, the only property that is patchable in the run is isArchiveEnabled

annatisch · 2018-08-23T22:24:15Z

.../resource-manager/Microsoft.ContainerRegistry/stable/2018-09-01/containerregistry_build.json

+          "Tasks"
+        ],
+        "description": "Returns a task with extended information that includes all secrets.",
+        "operationId": "Tasks_ListDetails",


This operation should be renamed to "Tasks_GetDetails" - as the response does not list anything.

We renamed it that way so that it can be used in ARM template

Thanks @mnltejaswini - the operation ID is only for the generated clients and does not impact the URL or ARM templates.

I see, ok then I will change the operation ID

annatisch · 2018-08-23T22:45:46Z

.../resource-manager/Microsoft.ContainerRegistry/stable/2018-09-01/containerregistry_build.json

+            "format": "int32"
+          },
+          {
+            "name": "$skipToken",


@mnltejaswini - I think what @johanste is saying is that you can probably remove this parameter because the generated SDKs already support retrieving the next page directly from nextLink - so unless there's a particular situation where a customer would need to extract the skipToken from nextLink, it's not really necessary.

Azure container registry build GA API specification and examples.

9287b2d

azuresdkci assigned annatisch Aug 9, 2018

azuresdkci requested a review from annatisch August 9, 2018 18:23

AutorestCI mentioned this pull request Aug 9, 2018

[AutoPR containerregistry/resource-manager] Azure container registry build feature GA API specification and examples. Azure/azure-sdk-for-ruby#1556

Closed

AutorestCI mentioned this pull request Aug 9, 2018

[AutoPR containerregistry/resource-manager] Azure container registry build feature GA API specification and examples. Azure/azure-sdk-for-java#2312

Merged

annatisch added the WaitForARMFeedback <valid label in PR review process> add this label when ARM review is required label Aug 9, 2018

annatisch requested a review from ravbhatnagar August 9, 2018 18:29

mnltejaswini added 2 commits August 9, 2018 14:15

Fix the Model validation errors in example files.

d56c86d

Removed secrets from Task properties as this is not yet supported.

f8c09be

Removed Alias and secrets property from task.

a3b1cb9

AutorestCI mentioned this pull request Aug 14, 2018

[AutoPR containerregistry/resource-manager] Azure container registry build feature GA API specification and examples. Azure/azure-sdk-for-ruby#1575

Merged

johanste reviewed Aug 14, 2018

View reviewed changes

Fixed the readme to the latest tag.

c8aea12

johanste reviewed Aug 14, 2018

View reviewed changes

separate the PUT and PATCH request payload

9f8daca

Matching the resource schema with that in the Registry swagger

70d7d7f

added more description to $filer, separated create and update paramet…

bc8b9e5

…ers to avoid conflict between required nd patchable properties.

Replaced GitCOmmitTrigger to SourceTrigger

38c5f01

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 Aug 22, 2018

Added isArchiveEnabled as part of Schedule Run request, renamed the a…

d501727

…rguments property to values for Task type requests and steps

mnltejaswini added 2 commits August 23, 2018 14:52

Removed the Required attribute for the source control auth properties…

bd17de8

… to fix the model validation failures.

Update readme.md to include tags for all languages

40798dd

AutorestCI mentioned this pull request Aug 23, 2018

[AutoPR containerregistry/resource-manager] Azure container registry build feature GA API specification and examples. Azure/azure-sdk-for-go#2532

Merged

annatisch requested changes Aug 23, 2018

View reviewed changes

Updated the operation ID as per the feedback

fd9d36e

annatisch approved these changes Aug 23, 2018

View reviewed changes

annatisch merged commit 4ed1dcd into Azure:master Aug 23, 2018

northtyphoon mentioned this pull request Oct 3, 2018

ACR 3.0 SDK (support ACR Build/Task GA Service) Azure/azure-sdk-for-net#4825

Merged

8 tasks

Azure container registry build feature GA API specification and examples. #3617

Azure container registry build feature GA API specification and examples. #3617

Conversation

mnltejaswini commented Aug 9, 2018 • edited Loading

PR information

Quality of Swagger

AutorestCI commented Aug 9, 2018 • edited Loading

Automation for azure-sdk-for-python

AutorestCI commented Aug 9, 2018 • edited Loading

Automation for azure-sdk-for-node

AutorestCI commented Aug 9, 2018 • edited Loading

Automation for azure-sdk-for-ruby

azuresdkci commented Aug 9, 2018

AutorestCI commented Aug 9, 2018 • edited Loading

Automation for azure-sdk-for-go

AutorestCI commented Aug 9, 2018 • edited Loading

Automation for azure-sdk-for-java

annatisch commented Aug 9, 2018

annatisch commented Aug 10, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johanste Aug 14, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mnltejaswini commented Aug 14, 2018

johanste commented Aug 14, 2018

mnltejaswini commented Aug 14, 2018

ravbhatnagar commented Aug 15, 2018

mnltejaswini commented Aug 15, 2018

mnltejaswini commented Aug 20, 2018

ravbhatnagar commented Aug 22, 2018

annatisch commented Aug 22, 2018

mnltejaswini commented Aug 23, 2018

mnltejaswini commented Aug 23, 2018 • edited Loading

annatisch commented Aug 23, 2018

mnltejaswini commented Aug 23, 2018

annatisch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mnltejaswini commented Aug 9, 2018 •

edited

Loading

AutorestCI commented Aug 9, 2018 •

edited

Loading

AutorestCI commented Aug 9, 2018 •

edited

Loading

AutorestCI commented Aug 9, 2018 •

edited

Loading

AutorestCI commented Aug 9, 2018 •

edited

Loading

AutorestCI commented Aug 9, 2018 •

edited

Loading

johanste Aug 14, 2018 •

edited

Loading

mnltejaswini commented Aug 23, 2018 •

edited

Loading