[merge on 4/20] add a copy image operation to Azure Container Registry #2719
Conversation
Automation for azure-sdk-for-nodeThe initial PR has been merged into your service PR: |
Automation for azure-libraries-for-javaThe initial PR has been merged into your service PR: |
Automation for azure-sdk-for-pythonThe initial PR has been merged into your service PR: |
Automation for azure-sdk-for-goA 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: |
olydis
added
the
WaitForARMFeedback
|
@ravbhatnagar new operation |
ravbhatnagar
left a comment
ravbhatnagar
left a comment
There was a problem hiding this comment.
@olydis - please feel free to merge based on response to these comments.
There was a problem hiding this comment.
can we make this a string enum? ex - "copyMode" propetrties where values are force etc.
There was a problem hiding this comment.
Created a new mode enum.
There was a problem hiding this comment.
Can we just call this copyImage?
There was a problem hiding this comment.
Renamed to importImage.
ravbhatnagar
added
ARMSignedOff
|
Couple of minor comments. @olydis - Please feel free to merge based on response to the 2 comments I raised above. |
nathana1
force-pushed
the
nathana/acr-copyimage-api
branch
from
ffa69db to
4e6be6f
Compare
There was a problem hiding this comment.
Please add
"x-ms-enum": {
"name": <please choose something meaningful>,
"modelAsString": true
}
There was a problem hiding this comment.
Please add "type": "object".
There was a problem hiding this comment.
@olydis We were instructed from the very first swagger 2016-06-27-preview version that this "type": "object" should be removed from here since it's redundant. We don't have it in all the versions since then, and looking at other swaggers such as Storage, Compute, etc. they don't have this either. Is this a new requirement? Or do we need to update other places if "type": "object" is now required?
There was a problem hiding this comment.
Yeeeeeah so somehow the idea that type: object is redundant was a false assumption we made for a long long time. Swagger follows JSON schema, and JSON says that something that doesn't have a type has any type (if you have stuff like properties, it is ignored unless you are looking at an object but, say, a number also matches such schema! Oops). Long story short, except for a handful of cases across all of Azure, stuff really has a type! No worries about the past Swaggers and also no worries about the future - our ancient code generation was created with the same false assumption so it behaves perfectly fine regardless of type or not. But external tools may take it more seriously, and ultimately our job is to provide valid Swaggers to our customers... and currently, if a PUT's body accepts a { "properties": { ... } } then that means customers are allowed to send a 42 over the wire rather than { ... }. We have a work item on our side to normalize the entire repo to have type: object where it belongs, but whenever I see it in a PR, I ask for it... but I don't expect you to spend more efforts on that since indeed we gave the wrong guidance for years!
There was a problem hiding this comment.
We use a global filter in swashbuckle to get rid of all the "type": "object" so I guess it's actually easier for us to consistently have it in this swagger, rather than having a special case for the new API/definitions, but we probably don't want to touch old swaggers as you also mentioned.
There was a problem hiding this comment.
ah sure, that makes perfect sense 👍
There was a problem hiding this comment.
Please add "type": "object".
There was a problem hiding this comment.
Also: Is the property required? Or could you specify {} as source of ImportImageParameters?
|
@nathana1 @ravbhatnagar What's the timeline for providing an example for the new operation? |
nathana1
force-pushed
the
nathana/acr-copyimage-api
branch
from
4e6be6f to
d3b9410
Compare
|
@olydis There's an ImportImage.json file included in the examples folder. Let me know if anything else is needed for an example. |
|
@nathana1 oh thanks! I blindly reported what our tooling said... which complained since you also need to hook the example into the Swagger. Please take a look at any other operation with regards to |
There was a problem hiding this comment.
Do we have no response payload at all?
There was a problem hiding this comment.
nope, just the return code
There was a problem hiding this comment.
NIT: registry -> container registry.
There was a problem hiding this comment.
Can we name this importImageParameters? importParameters sounds too general.
There was a problem hiding this comment.
Looking at it I think it was too redundant (it was on an operation named importImage), so I've renamed it to parameters.
There was a problem hiding this comment.
NIT: registry -> container registry.
There was a problem hiding this comment.
I wonder if sourceRepository, sourceTag, and sourceManifestDigest can be combined to one single parameter such as sourceImage which can be the following format:
hello-world->hello-world:latest.- `hello-world:v21
hello-world@sha256:abc123
This will be consistent with docker and avoid the mentioning of digest is exclusive with tag.
/cc @sajayantony
There was a problem hiding this comment.
made an update to this and updated the samples
There was a problem hiding this comment.
Can we add something like default: NoForce rather than mentioning the default value in description?
NIT: remove the extra space in overwritten. When. There are extra spaces in other descriptions too.
nathana1
force-pushed
the
nathana/acr-copyimage-api
branch
from
d3b9410 to
f7bb60b
Compare
nathana1
force-pushed
the
nathana/acr-copyimage-api
branch
from
f7bb60b to
57b80df
Compare
|
This is live in dogfood. We are trying to go live in production in about a week or so. |
There was a problem hiding this comment.
Repository name has to be lower case so I would lower case the example name sourceRepository here to make it more realistic. The same applies to other cases.
- adds an operation to support copying a Docker image from one registry to another - supports copying to multiple places in the target registry - only one source and one target registry are supported
nathana1
force-pushed
the
nathana/acr-copyimage-api
branch
from
57b80df to
8c306b9
Compare
|
@nathana1 Thanks for the response! Please ping me once you are live in public since that's when this will be merged 🙂 BTW, please don't force push, I now don't see which things you changed based on review feedback, so it's super hard to review! I of course trust that nothing else was changed since ARM signoff and such, but it's less obvious what to look at when I wanna do an iteration on this PR. 😉 |
|
@nathana1 @djyou any news? Do you have a date for going live in public? |
|
We're in the process of rolling this out in prod right now (in canary so far) so it should be everywhere in about a week. Unfortunately I think the regex for that particular data would be too complicated and the documentation is the best way to describe the value. |
|
Alright, thanks for your feedback! I'll mark this as "OK to merge" and add a date of 4/20 to the PR title - please reach out (or edit the PR title) if you want to change that date 🙂 |
olydis
changed the title
olydis
added
Approved-OkToMerge
|
@jianghaolu Handing this one off to you 😉 Review is complete, as you can see in the very last comments, this just has to be merged once the service is live in public (see title for estimated date). |
|
It's 4/20. Any concerns before I merge this? |
7 participants
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
api-versionin the path should match theapi-versionin the spec).Quality of Swagger