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.

Sign up for GitHub

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

Jump to bottom

Arch board review feedback for ACR #21913

Merged
merged 5 commits into from
Jun 1, 2021
Merged

Arch board review feedback for ACR #21913

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
b6674a1
Update ACR changes
pallavit May 24, 2021
4fd9bb2
Update the swagger to the new values.
pallavit May 24, 2021
9df866a
read me update
pallavit May 24, 2021
dbd6fd4
Add support for ACR beta 3
pallavit May 27, 2021
78722bb
Incorporate CR comments
pallavit Jun 1, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
69 changes: 35 additions & 34 deletions sdk/containerregistry/azure-containers-containerregistry/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,20 +39,20 @@ More information at [Azure Container Registry portal][container_registry_create_

<!-- embedme ./src/samples/java/com/azure/containers/containerregistry/ReadmeSamples.java#L29-L33 -->
```Java
ContainerRegistryClient client = new ContainerRegistryClientBuilder()
.endpoint(endpoint)
.credential(credential)
.buildClient();
}
DefaultAzureCredential credential = new DefaultAzureCredentialBuilder().build();
ContainerRegistryClient client = new ContainerRegistryClientBuilder()
.endpoint(endpoint)
.credential(credential)
.buildClient();
```

<!-- embedme ./src/samples/java/com/azure/containers/containerregistry/ReadmeSamples.java#L37-L41 -->
```Java
ContainerRegistryAsyncClient client = new ContainerRegistryClientBuilder()
.endpoint(endpoint)
.credential(credential)
.buildAsyncClient();
}
DefaultAzureCredential credential = new DefaultAzureCredentialBuilder().build();
ContainerRegistryAsyncClient client = new ContainerRegistryClientBuilder()
.endpoint(endpoint)
.credential(credential)
.buildAsyncClient();
```

For more information on using AAD with Azure Container Registry, please see the service's [Authentication Overview](https://docs.microsoft.com/azure/container-registry/container-registry-authentication).
Expand All @@ -63,14 +63,14 @@ The user must use this setting on a registry that has been enabled for anonymous
In this mode, the user can only call listRepositoryNames method and its overload. All the other calls will fail.
For more information please read [Anonymous Pull Access](https://docs.microsoft.com/azure/container-registry/container-registry-faq#how-do-i-enable-anonymous-pull-access)

<!-- embedme ./src/samples/java/com/azure/containers/containerregistry/ReadmeSamples.java#L70-L72 -->
<!-- embedme ./src/samples/java/com/azure/containers/containerregistry/ReadmeSamples.java#L71-L73 -->
```Java
ContainerRegistryClient client = new ContainerRegistryClientBuilder()
.endpoint(endpoint)
.buildClient();
```

<!-- embedme ./src/samples/java/com/azure/containers/containerregistry/ReadmeSamples.java#L76-L78 -->
<!-- embedme ./src/samples/java/com/azure/containers/containerregistry/ReadmeSamples.java#L77-L79 -->
```Java
ContainerRegistryAsyncClient client = new ContainerRegistryClientBuilder()
.endpoint(endpoint)
Expand Down Expand Up @@ -105,7 +105,7 @@ For more information please see [Container Registry Concepts](https://docs.micro

Iterate through the collection of repositories in the registry.

<!-- embedme ./src/samples/java/com/azure/containers/containerregistry/ReadmeSamples.java#L44-L50 -->
<!-- embedme ./src/samples/java/com/azure/containers/containerregistry/ReadmeSamples.java#L45-L51 -->
```Java
DefaultAzureCredential credential = new DefaultAzureCredentialBuilder().build();
ContainerRegistryClient client = new ContainerRegistryClientBuilder()
Expand All @@ -118,16 +118,16 @@ client.listRepositoryNames().forEach(repository -> System.out.println(repository

### List tags with anonymous access

<!-- embedme ./src/samples/java/com/azure/containers/containerregistry/ReadmeSamples.java#L137-L148 -->
<!-- embedme ./src/samples/java/com/azure/containers/containerregistry/ReadmeSamples.java#L138-L149 -->
```Java
ContainerRegistryClient anonymousClient = new ContainerRegistryClientBuilder()
.endpoint(endpoint)
.buildClient();

RegistryArtifact image = anonymousClient.getArtifact(repositoryName, digest);
PagedIterable<ArtifactTagProperties> tags = image.listTags();
PagedIterable<ArtifactTagProperties> tags = image.listTagProperties();

System.out.printf(String.format("%s has the following aliases:", image.getFullyQualifiedName()));
System.out.printf(String.format("%s has the following aliases:", image.getFullyQualifiedReference()));

for (ArtifactTagProperties tag : tags) {
System.out.printf(String.format("%s/%s:%s", anonymousClient.getEndpoint(), repositoryName, tag.getName()));
Expand All @@ -136,7 +136,7 @@ for (ArtifactTagProperties tag : tags) {

### Set artifact properties

<!-- embedme ./src/samples/java/com/azure/containers/containerregistry/ReadmeSamples.java#L116-L129 -->
<!-- embedme ./src/samples/java/com/azure/containers/containerregistry/ReadmeSamples.java#L117-L130 -->
```Java
TokenCredential defaultCredential = new DefaultAzureCredentialBuilder().build();

Expand All @@ -156,7 +156,7 @@ image.updateTagProperties(

### Delete Images

<!-- embedme ./src/samples/java/com/azure/containers/containerregistry/ReadmeSamples.java#L82-L110 -->
<!-- embedme ./src/samples/java/com/azure/containers/containerregistry/ReadmeSamples.java#L83-L111 -->
```Java
TokenCredential defaultCredential = new DefaultAzureCredentialBuilder().build();

Expand All @@ -171,8 +171,8 @@ for (String repositoryName : client.listRepositoryNames()) {

// Obtain the images ordered from newest to oldest
PagedIterable<ArtifactManifestProperties> imageManifests =
repository.listManifests(
ManifestOrderBy.LAST_UPDATED_ON_DESCENDING,
repository.listManifestProperties(
ArtifactManifestOrderBy.LAST_UPDATED_ON_DESCENDING,
Context.NONE);

imageManifests.stream().skip(imagesCountToKeep)
Expand All @@ -190,7 +190,7 @@ for (String repositoryName : client.listRepositoryNames()) {
```

### Delete repository with anonymous access throws
<!-- embedme ./src/samples/java/com/azure/containers/containerregistry/ReadmeSamples.java#L152-L164 -->
<!-- embedme ./src/samples/java/com/azure/containers/containerregistry/ReadmeSamples.java#L153-L165 -->
```Java
final String endpoint = getEndpoint();
final String repositoryName = getRepositoryName();
Expand All @@ -202,7 +202,7 @@ ContainerRegistryClient anonymousClient = new ContainerRegistryClientBuilder()
try {
anonymousClient.deleteRepository(repositoryName);
System.out.println("Unexpected Success: Delete is not allowed on anonymous access");
} catch (Exception ex) {
} catch (ClientAuthenticationException ex) {
System.out.println("Expected exception: Delete is not allowed on anonymous access");
}
```
Expand All @@ -212,19 +212,20 @@ try {
All container registry service operations will throw a
[HttpResponseException][HttpResponseException] on failure.

<!-- embedme ./src/samples/java/com/azure/containers/containerregistry/ReadmeSamples.java#L56-L66 -->
<!-- embedme ./src/samples/java/com/azure/containers/containerregistry/ReadmeSamples.java#L56-L67 -->
```Java
DefaultAzureCredential credential = new DefaultAzureCredentialBuilder().build();
ContainerRepository containerRepository = new ContainerRegistryClientBuilder()
.endpoint(endpoint)
.credential(credential)
.buildClient()
.getRepository(repositoryName);
try {
containerRepository.getProperties();
} catch (HttpResponseException exception) {
// Do something with the exception.
}
public void getPropertiesThrows() {
DefaultAzureCredential credential = new DefaultAzureCredentialBuilder().build();
ContainerRepository containerRepository = new ContainerRegistryClientBuilder()
.endpoint(endpoint)
.credential(credential)
.buildClient()
.getRepository(repositoryName);
try {
containerRepository.getProperties();
} catch (HttpResponseException exception) {
// Do something with the exception.
}
```

## Next steps
Expand Down
30 changes: 24 additions & 6 deletions ...ry/src/main/java/com/azure/containers/containerregistry/ContainerRegistryAsyncClient.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,10 @@
*
* <p><strong>Instantiating an asynchronous Container Registry client</strong></p>
*
* {@codesnippet com.azure.containers.containerregistry.async.repository.instantiation}
* {@codesnippet com.azure.containers.containerregistry.ContainerRegistryAsyncClient.instantiation}
*
* <p><strong>Instantiating an asynchronous Container Registry client using a custom pipeline</strong></p>
* {@codesnippet com.azure.containers.containerregistry.ContainerRegistryAsyncClient.pipeline.instantiation}
*
* <p>View {@link ContainerRegistryClientBuilder this} for additional ways to construct the client.</p>
*
Expand Down Expand Up @@ -68,6 +71,10 @@ public String getEndpoint() {
/**
* List all the repository names in this registry.
*
* <p><strong>List repository names in the registry.</strong></p>
*
* {@codesnippet com.azure.containers.containerregistry.ContainerRegistryAsyncClient.listRepositoryNames}
*
* @return list of repository names.
* @throws ClientAuthenticationException thrown if the client's credentials do not have access to modify the namespace.
*/
Expand Down Expand Up @@ -112,6 +119,10 @@ Mono<PagedResponse<String>> listRepositoryNamesNextSinglePageAsync(String nextLi
/**
* Delete the repository identified by 'repositoryName'.
*
* <p><strong>Delete a repository in the registry.</strong></p>
*
* {@codesnippet com.azure.containers.containerregistry.ContainerRegistryAsyncClient.deleteRepositoryWithResponse#String}
*
* @param repositoryName Name of the repository (including the namespace).
* @return the completion.
* @throws ClientAuthenticationException thrown if the client's credentials do not have access to modify the namespace.
Expand Down Expand Up @@ -144,8 +155,11 @@ Mono<Response<Void>> deleteRepositoryWithResponse(String repositoryName, Context
/**
* Delete the repository identified by 'repositoryName'.
*
* <p><strong>Delete a repository in the registry.</strong></p>
* {@codesnippet com.azure.containers.containerregistry.ContainerRegistryAsyncClient.deleteRepository#String}
*
* @param repositoryName Name of the image (including the namespace).
* @return deleted repository properties.
* @return the completion stream.
* @throws ClientAuthenticationException thrown if the client's credentials do not have access to modify the namespace.
* @throws NullPointerException thrown if the 'repositoryName' is null.
* @throws IllegalArgumentException thrown if 'repositoryName' is empty.
Expand All @@ -162,6 +176,9 @@ Mono<Void> deleteRepository(String repositoryName, Context context) {
/**
* Creates a new instance of {@link ContainerRepositoryAsync} object for the specified repository.
*
* <p><strong>Create an instance of ContainerRepositoryAsync helper type</strong></p>
* {@codesnippet com.azure.containers.containerregistry.containeregistryasyncclient.getRepository}
*
* @param repositoryName Name of the repository to reference.
* @return A new {@link ContainerRepositoryAsync} for the desired repository.
* @throws NullPointerException if 'repositoryName' is null.
Expand All @@ -174,13 +191,14 @@ public ContainerRepositoryAsync getRepository(String repositoryName) {
/**
* Creates a new instance of {@link RegistryArtifactAsync} object for the specified artifact.
*
* <p><strong>Create an instance of RegistryArtifactAsync helper type</strong></p>
* {@codesnippet com.azure.containers.containerregistry.containeregistryasyncclient.getArtifact}
*
* @param repositoryName Name of the repository to reference.
* @param digest Either a tag or digest that uniquely identifies the artifact.
* @return A new {@link RegistryArtifactAsync RegistryArtifactAsync} for the desired repository.
* @throws NullPointerException if 'repositoryName' is null.
* @throws IllegalArgumentException if 'repositoryName' is empty.
* @throws NullPointerException if 'digest' is null.
* @throws IllegalArgumentException if 'digest' is empty.
* @throws NullPointerException if 'repositoryName' or 'digest' is null.
Copy link
Member

@alzimmermsft alzimmermsft May 28, 2021 •
edited by pallavit
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the parameter names we generally use {@code }, ex

Suggested change
* @throws NullPointerException if 'repositoryName' or 'digest' is null.
* @throws NullPointerException if {@code repositoryName} or {@code digest} is null.

As Javadoc will stylize them to be more obvious. #Resolved

* @throws IllegalArgumentException if 'repositoryName' or 'digest 'is empty.
*/
public RegistryArtifactAsync getArtifact(String repositoryName, String digest) {
return new RegistryArtifactAsync(repositoryName, digest, httpPipeline, endpoint, apiVersion);
Expand Down
26 changes: 24 additions & 2 deletions ...egistry/src/main/java/com/azure/containers/containerregistry/ContainerRegistryClient.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,10 @@
* that can be used to perform operations on repository and artifacts.
*
* <p><strong>Instantiating a synchronous Container Registry client</strong></p>
* {@codesnippet com.azure.containers.containerregistry.ContainerRegistryClient.instantiation}
*
* {@codesnippet com.azure.containers.containerregistry.repository.instantiation}
* <p><strong>Instantiating a synchronous Container Registry client with custom pipeline</strong></p>
* {@codesnippet com.azure.containers.containerregistry.ContainerRegistryClient.pipeline.instantiation}
*
* <p>View {@link ContainerRegistryClientBuilder this} for additional ways to construct the client.</p>
*
Expand All @@ -44,7 +46,11 @@ public String getEndpoint() {
/**
* List all the repository names in this registry.
*
* @return list of repositories.
* <p><strong>List the repository names in the registry.</strong></p>
*
* {@codesnippet com.azure.containers.containerregistry.ContainerRegistryClient.listRepositoryNames}
*
* @return list of repository names.
* @throws ClientAuthenticationException thrown if the client credentials do not have access to perform this operation.
*/
@ServiceMethod(returns = ReturnType.COLLECTION)
Expand All @@ -55,6 +61,9 @@ public PagedIterable<String> listRepositoryNames() {
/**
* List all the repository names in this registry.
*
* <p><strong>List the repository names in the registry.</strong></p>
* {@codesnippet com.azure.containers.containerregistry.listRepositoryNames#Context}
*
* @param context The context to associate with this operation.
* @return list of repositories.
* @throws ClientAuthenticationException thrown if the client credentials do not have access to perform this operation.
Expand All @@ -67,6 +76,9 @@ public PagedIterable<String> listRepositoryNames(Context context) {
/**
* Delete the repository identified by 'repositoryName'.
*
* <p><strong>Delete a repository in the registry.</strong></p>
* {@codesnippet com.azure.containers.containerregistry.ContainerRegistryClient.deleteRepository#String}
*
* @param repositoryName Name of the repository (including the namespace).
* @throws ClientAuthenticationException thrown if the client's credentials do not have access to modify the namespace.
* @throws NullPointerException thrown if the 'repositoryName' is null.
Expand All @@ -80,6 +92,9 @@ public void deleteRepository(String repositoryName) {
/**
* Delete the repository identified by 'repositoryName'.
*
* <p><strong>Delete a repository in the registry.</strong></p>
* {@codesnippet com.azure.containers.containerregistry.ContainerRegistryClient.deleteRepositoryWithResponse#String-Context}
*
* @param repositoryName Name of the repository (including the namespace).
* @param context The context to associate with this operation.
* @return Completion response.
Expand All @@ -95,6 +110,9 @@ public Response<Void> deleteRepositoryWithResponse(String repositoryName, Contex
/**
* Creates a new instance of {@link ContainerRepository} object for the specified repository.
*
* <p><strong>Create a ContainerRegistry helper instance.</strong></p>
* {@codesnippet com.azure.containers.containerregistry.getRepository}
*
* @param repositoryName Name of the repository to reference.
* @return A new {@link ContainerRepository} for the desired repository.
* @throws NullPointerException if 'repositoryName' is null.
Expand All @@ -107,6 +125,10 @@ public ContainerRepository getRepository(String repositoryName) {
/**
* Creates a new instance of {@link RegistryArtifact} object for the specified artifact.
*
* <p><strong>Create a RegistryArtifact helper instance.</strong></p>
* {@codesnippet com.azure.containers.containerregistry.getArtifact}
*
*
* @param repositoryName Name of the repository to reference.
* @param digest Either a tag or digest that uniquely identifies the artifact.
* @return A new {@link RegistryArtifact} object for the desired repository.
Expand Down
39 changes: 27 additions & 12 deletions .../src/main/java/com/azure/containers/containerregistry/ContainerRegistryClientBuilder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,13 +30,11 @@
* the desired client.
*
* <p>The client needs the service endpoint of the Azure Container Registry and Azure access credentials.
* <p><strong>Instantiating an asynchronous {@link ContainerRegistryAsyncClient}</strong></p>
* <p><strong>Instantiating an asynchronous Container Registry client</strong></p>
* {@codesnippet com.azure.containers.containerregistry.ContainerRegistryAsyncClient.instantiation}
*
* {@codesnippet com.azure.containers.containerregistry.async.repository.instantiation}
*
* <p><strong>Instantiating a synchronous Configuration Client</strong></p>
*
* {@codesnippet com.azure.containers.containerregistry.repository.instantiation}
* <p><strong>Instantiating a synchronous Container Registry client</strong></p>
* {@codesnippet com.azure.containers.containerregistry.ContainerRegistryClient.instantiation}
*
* <p>Another way to construct the client is using a {@link HttpPipeline}. The pipeline gives the client an
* authenticated way to communicate with the service but it doesn't contain the service endpoint. Set the pipeline with
Expand All @@ -48,13 +46,11 @@
* would need to provide implementation for this policy as well.
* For more information please see <a href="https://github.com/Azure/acr/blob/main/docs/AAD-OAuth.md"> Azure Container Registry Authentication </a>.</p>
*
* <p><strong>Instantiating an asynchronous {@link ContainerRegistryAsyncClient}</strong></p>
*
* {@codesnippet com.azure.containers.containerregistry.async.repository.pipeline.instantiation}
*
* <p><strong>Instantiating a synchronous Configuration Client</strong></p>
* <p><strong>Instantiating an asynchronous Container Registry client using a custom pipeline</strong></p>
* {@codesnippet com.azure.containers.containerregistry.ContainerRegistryAsyncClient.pipeline.instantiation}
*
* {@codesnippet com.azure.containers.containerregistry.repository.pipeline.instantiation}
* <p><strong>Instantiating a synchronous Container Registry client with custom pipeline</strong></p>
* {@codesnippet com.azure.containers.containerregistry.ContainerRegistryClient.pipeline.instantiation}
*
*
* @see ContainerRegistryAsyncClient
Expand Down Expand Up @@ -88,6 +84,7 @@ public final class ContainerRegistryClientBuilder {
private HttpLogOptions httpLogOptions;
private RetryPolicy retryPolicy;
private ContainerRegistryServiceVersion version;
private String authenticationScope;

/**
* Sets the service endpoint for the Azure Container Registry instance.
Expand All @@ -107,6 +104,23 @@ public ContainerRegistryClientBuilder endpoint(String endpoint) {
return this;
}

/**
* Sets the authentication scope to be used for getting AAD credentials.
*
Copy link
Member

@alzimmermsft alzimmermsft May 28, 2021 •
edited by pallavit
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: <p> to create separation from the initial statement and additional explanation paragraphs/sentences. #Resolved

* NOTE - This is a temporary property that is added into the system until the service
* exposes this directly via the challenge based auth scheme.
* If this property is not provided then by default Azure public scope is used for authentication.
*
* Example:- For Azure public cloud this value is same as AzureEnvironment.Azure.managementEndpoint().
Copy link
Member

@alzimmermsft alzimmermsft May 28, 2021 •
edited by pallavit
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could this link to a static resource with further explanation? Right now, as a user if I'm new to Azure SDKs, or Azure libraries in general, where would I go to understand this further based on this example? #Resolved

*
* @param authenticationScope ARM management scope associated with the given registry.
* @return The updated {@link ContainerRegistryClientBuilder} object.
*/
public ContainerRegistryClientBuilder authenticationScope(String authenticationScope) {
this.authenticationScope = authenticationScope;
return this;
}

/**
* Sets the {@link TokenCredential} used to authenticate REST API calls.
*
Expand Down Expand Up @@ -282,6 +296,7 @@ private HttpPipeline getHttpPipeline() {
this.configuration,
this.retryPolicy,
this.credential,
this.authenticationScope,
this.perCallPolicies,
this.perRetryPolicies,
this.httpClient,
Expand Down
Loading