Skip to content
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

Updates to ACR Swagger file to improve Track 2 code generation #2478

Closed
13 of 21 tasks
annelo-msft opened this issue Feb 25, 2021 · 5 comments
Closed
13 of 21 tasks

Updates to ACR Swagger file to improve Track 2 code generation #2478

annelo-msft opened this issue Feb 25, 2021 · 5 comments
Assignees
Labels
Client This issue points to a problem in the data-plane of the library. Container Registry
Milestone

Comments

@annelo-msft
Copy link
Member

annelo-msft commented Feb 25, 2021

We would like the generator to write as much of the ACR client library code as possible, across all languages. To achieve this, we'll make as many of the customizations we'll need for Track 2 in the Swagger layer.

At a high level, there are two categories of swagger changes:

  1. Ones we know we'll need, regardless of what the final, architect-approved API design will be
  2. Ones we should only make once we have a reasonably good sense of the approved design

Some items included in 1 are:

Preview 1

  • Add x-ms-pageable blocks for generating pageable methods.
    • /acr/v1/_catalog GetRepositories()
    • /acr/v1/{name}/_manifests GetRegistryArtifacts()
    • /acr/v1/{name}/_tags GetTags()
    • Added. However, the link value returned by the service won't work directly when passing to Get*Next() method, still need code in convenience layer in each language to make it work.

Preview 2

  • Add "monolithic upload in one request" operation to the swagger file so we get it in code-gen (see "POST INITIATE BLOB UPLOAD" under Initiate Blob Upload for details -- the service supports it, but it is not in the swagger file).
  • Investigate: is specifying Range header as input parameter incorrect, where Content-Range header should be used for input and Range used for output instead?

It would be great to look into the autorest warnings to see if there are more of these, or otherwise investigate if there are more.

Some items in 2 (that we should wait to do) are:
Preview 1

  • Update tags and operationId blocks once we've decided on client design so we get the right client and names from code gen
    • Looks already done by Anne.
  • Update model type names and shapes once design has been finalized
    • update model type names
    • update property names
    • flatten nested attributes properties
  • mark non-nullable fields as required
  • fix /acr/v1/{name}/_manifests/{reference} should only take digest
  • DELETE /v2/{name}/manifests/{reference} should only take digest
  • /acr/v1/{name}/_tags/{reference} should only take tag it is already using a TagReference
  • ManifestsDeleted and TagsDeleted should be get-only
  • Attributes.MediaType and Attributes.ConfigMediaType should be internal
  • TagAttributesBase and ManifestAttributesBase should be internal
    • Marked. Although other language don't make use of internal. Marking them internal also prevents C# generated clients to be public because of incompatible accessibility.
  • tags should be array of TagProperties, and manifests should be array of RegistryArtifactProperties
    • We don't have a good solution to achieve this via transformation. We will have to do this in convenience layer in each language.

Since the ACR team is using this file to generate their Track 1 library, it would be ideal not to make modifications to this one, but instead create a new one like Search did here: https://github.com/Azure/azure-rest-api-specs/tree/master/specification/search/data-plane

@annelo-msft annelo-msft added Client This issue points to a problem in the data-plane of the library. Container Registry labels Feb 25, 2021
@annelo-msft annelo-msft added this to the [2021] April milestone Feb 25, 2021
@annelo-msft
Copy link
Member Author

@AlexGhiondea FYI

@AlexGhiondea
Copy link
Contributor

@jeremymeng could you take a look at this one?

@jeremymeng
Copy link
Member

Looking

@annelo-msft
Copy link
Member Author

These can wait until Preview 2:

  • Add "monolithic upload in one request" operation to the swagger file so we get it in code-gen (see "POST INITIATE BLOB UPLOAD" under Initiate Blob Upload for details -- the service supports it, but it is not in the swagger file).
  • Investigate: is specifying Range header as input parameter incorrect, where Content-Range header should be used for input and Range used for output instead?

@jeremymeng
Copy link
Member

Closing this. Remaining work is tracked by #2633 and #2634.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Client This issue points to a problem in the data-plane of the library. Container Registry
Projects
None yet
Development

No branches or pull requests

3 participants