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

Container registry/blob push pull support #20529

Merged
merged 57 commits into from
Apr 2, 2022

Conversation

timovv
Copy link
Member

@timovv timovv commented Feb 24, 2022

Packages impacted by this PR

  • @azure/container-registry

Issues associated with this PR

Are there test cases added in this PR? (If not, why?)

Yes.

Checklists

  • Added impacted package name to the issue description
  • Does this PR needs any fixes in the SDK Generator?** (If so, create an Issue in the Autorest/typescript repository and link it here)
  • Added a changelog (if necessary)

@timovv timovv requested a review from jeremymeng February 24, 2022 22:36
@ghost ghost added the Container Registry label Feb 24, 2022
@azure-sdk
Copy link
Collaborator

API changes have been detected in @azure/container-registry. You can review API changes here

@azure-sdk
Copy link
Collaborator

API changes have been detected in @azure/container-registry. You can review API changes here

@azure-sdk
Copy link
Collaborator

API changes have been detected in @azure/container-registry. You can review API changes here

@azure-sdk
Copy link
Collaborator

API changes have been detected in @azure/container-registry. You can review API changes here

@timovv timovv changed the title WIP: Container registry/blob push pull support API changes WIP: Container registry/blob push pull support Mar 14, 2022
@timovv timovv changed the title WIP: Container registry/blob push pull support Container registry/blob push pull support Mar 14, 2022
@timovv timovv force-pushed the container-registry/blob-push-pull-support branch from 1ae7432 to 9372a95 Compare March 29, 2022 22:46
Copy link
Member

@jeremymeng jeremymeng left a 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 left some comments, but they can wait.

@azure-sdk
Copy link
Collaborator

API change check for @azure/container-registry

API changes have been detected in @azure/container-registry. You can review API changes here

Copy link
Member

@xirzec xirzec left a comment

Choose a reason for hiding this comment

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

A few more comments on some things I didn't notice before. I think the public shape looks great modulo the Options interface for the new client.

} else if (typeof manifest === "function") {
manifestBody = await readStreamToEnd(manifest());
} else {
const serialized = serializer.serialize(Mappers.OCIManifest, manifest);
Copy link
Member

Choose a reason for hiding this comment

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

interesting, I don't think we've had to do this in convenience before. I wonder if this is a pattern we should think about how to generalize more.

Comment on lines 44 to 50
function withRequiredProperty<T, U extends keyof T>(property: U, obj: T): T & Required<Pick<T, U>> {
if (!Object.prototype.hasOwnProperty.call(obj, property)) {
throw new RestError(`Expected property ${property} to be defined.`);
}

return obj as T & Required<Pick<T, U>>;
}
Copy link
Member

@xirzec xirzec Apr 1, 2022

Choose a reason for hiding this comment

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

Small preference for

Suggested change
function withRequiredProperty<T, U extends keyof T>(property: U, obj: T): T & Required<Pick<T, U>> {
if (!Object.prototype.hasOwnProperty.call(obj, property)) {
throw new RestError(`Expected property ${property} to be defined.`);
}
return obj as T & Required<Pick<T, U>>;
}
function assertRequiredProperty<T, U extends keyof T>(property: U, obj: T): asserts obj is T & Required<Pick<T, U>> {
if (!Object.prototype.hasOwnProperty.call(obj, property)) {
throw new RestError(`Expected property ${property} to be defined.`);
}
}

so you can use it like

const result = await this.client.containerRegistry.createManifest(
    this.repositoryName,
    tagOrDigest,
    manifestBody,
    { contentType: KnownManifestMediaType.OciManifestMediaType, ...updatedOptions }
  )
);
assertRequiredProperty("dockerContentDigest", result);

return { digest: result.dockerContentDigest };

Mostly because nesting the 'await' inside of a method call seems like an easy way to forget to do it later, and then we're asserting on a Promise object instead of the real result.

Copy link
Member

@xirzec xirzec left a comment

Choose a reason for hiding this comment

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

Thanks for the updates!

@timovv timovv merged commit 69448ed into Azure:main Apr 2, 2022
azure-sdk pushed a commit to azure-sdk/azure-sdk-for-js that referenced this pull request Sep 1, 2022
Adding readme.java.md for agrifood (Azure#20529)

* Adding readme.java.md for agrifood

* update

* Update readme.java.md

* Update readme.java.md

Co-authored-by: Weidong Xu <weidxu@microsoft.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Container Registry] OCI blob push/pull support
4 participants