[ContainerRegistry] beta.4 minor fixes #15722
Conversation
- Improve ref doc for listing methods.
as `isNode` is only used in tests. Also re-organize helper code.
sdk/containerregistry/container-registry/src/containerRegistryClient.ts
Outdated
Show resolved
Hide resolved
sdk/containerregistry/container-registry/src/containerRepository.ts
Outdated
Show resolved
Hide resolved
| @@ -84,7 +85,6 @@ | |||
| "@azure/core-rest-pipeline": "1.1.0-beta.4", | |||
| "@azure/core-paging": "^1.1.1", | |||
| "@azure/core-tracing": "1.0.0-preview.11", | |||
| "@azure/core-util": "^1.0.0-beta.1", | |||
There was a problem hiding this comment.
I'm moving Key Vault to corev2 so figured it'll be useful info for me too - is there a reason not to use core-util? Like should I also make local copies of the methods I need from it? I think there's only like 2 functions there.
There was a problem hiding this comment.
core-util is till in beta and we still want some investigation. I am copying code for now so ACR GA isn't blocked on core-util beta version. We can move to core-util after it is in GA.
| @@ -208,7 +208,7 @@ export interface RegistryArtifact { | |||
| listTagProperties(options?: ListTagPropertiesOptions): PagedAsyncIterableIterator<ArtifactTagProperties>; | |||
| readonly registryEndpoint: string; | |||
| readonly repositoryName: string; | |||
| updateManifestProperties(options?: UpdateManifestPropertiesOptions): Promise<ArtifactManifestProperties>; | |||
| updateManifestProperties(options: UpdateManifestPropertiesOptions): Promise<ArtifactManifestProperties>; | |||
There was a problem hiding this comment.
Why are we making this non-optional when all of the keys are optional? Do one of the props have to be true?
There was a problem hiding this comment.
It doesn't make sense to not pass anything to the update method. The properties are optional to allow update one of them without having to specify all of them. Is it better to make it optional then no-op when nothing/empty object is passed in?
There was a problem hiding this comment.
Yeah, I think you're right to make this required, just as a hint that they should do something.
As an aside, I spent some time getting TS magic for 'at least one of these properties should be on this object':
type AtLeastOne<Type> =
{
[Property in keyof Type]: Required<Pick<Type, Property>> & Partial<Type>
}[keyof Type]It works and does the right thing, but I think it's overkill for this. Maybe it'll be useful in a different place.
Co-authored-by: Maor Leger <maorleger@users.noreply.github.com>
| @@ -208,7 +208,7 @@ export interface RegistryArtifact { | |||
| listTagProperties(options?: ListTagPropertiesOptions): PagedAsyncIterableIterator<ArtifactTagProperties>; | |||
| readonly registryEndpoint: string; | |||
| readonly repositoryName: string; | |||
| updateManifestProperties(options?: UpdateManifestPropertiesOptions): Promise<ArtifactManifestProperties>; | |||
| updateManifestProperties(options: UpdateManifestPropertiesOptions): Promise<ArtifactManifestProperties>; | |||
There was a problem hiding this comment.
Yeah, I think you're right to make this required, just as a hint that they should do something.
As an aside, I spent some time getting TS magic for 'at least one of these properties should be on this object':
type AtLeastOne<Type> =
{
[Property in keyof Type]: Required<Pick<Type, Property>> & Partial<Type>
}[keyof Type]It works and does the right thing, but I think it's overkill for this. Maybe it'll be useful in a different place.
UpdateManifestPropertiesOptionsshould not be optionalgetRepository()@azure/core-util