Skip to content
This repository has been archived by the owner on Apr 7, 2024. It is now read-only.

feat: expose registry name mapping methods #76

Merged
merged 1 commit into from
Jun 19, 2023
Merged

feat: expose registry name mapping methods #76

merged 1 commit into from
Jun 19, 2023

Conversation

wangxiaoxuan273
Copy link
Collaborator

Resolves #72

@codecov-commenter
Copy link

codecov-commenter commented Jun 12, 2023

Codecov Report

Merging #76 (4e86073) into main (bf5244c) will decrease coverage by 0.28%.
The diff coverage is 100.00%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

@@            Coverage Diff             @@
##             main      #76      +/-   ##
==========================================
- Coverage   82.73%   82.46%   -0.28%     
==========================================
  Files           6        6              
  Lines         388      382       -6     
==========================================
- Hits          321      315       -6     
  Misses         45       45              
  Partials       22       22              
Impacted Files Coverage Δ
registry.go 81.25% <100.00%> (-2.09%) ⬇️

Copy link
Member

@TerryHowe TerryHowe left a comment

Choose a reason for hiding this comment

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

/lgtm

registry.go Outdated
if reg == "" {
return auth.EmptyCredential, nil
}
return store.Get(ctx, reg)
}
}

func mapStoreRegistryName(registry string) string {
func MapRegistryNameDockerIo(registry string) string {
Copy link
Contributor

Choose a reason for hiding this comment

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

The method name is not meaningful.

Copy link
Contributor

Choose a reason for hiding this comment

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

This method maps a registry name to a key for credential store. Therefore, func ServerAddressFromRegistry(registry string) string with documentation is preferable.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

changed accordingly.

registry.go Outdated
@@ -89,7 +89,7 @@ func mapStoreRegistryName(registry string) string {
return registry
}

func mapAuthenticationRegistryName(hostname string) string {
func MapRegistryNameRegistry1DockerIo(hostname string) string {
Copy link
Contributor

Choose a reason for hiding this comment

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

The method name is not meaningful.

Copy link
Contributor

Choose a reason for hiding this comment

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

This method maps a host name to a key for credential store. Therefore, func ServerAddressFromHostname(hostname string) string with documentation is preferable.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

changed accordingly.

registry.go Outdated
// ServerAddressFromRegistry maps a registry to a server address, which is used as
// a key for credentials store. The Docker CLI expects that the credentials of
// the registry 'docker.io' will be added under the key "https://index.docker.io/v1/".
// See: https://github.com/moby/moby/blob/v24.0.0-beta.2/registry/config.go#L25-L48
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: v24.0.2 is out.

Suggested change
// See: https://github.com/moby/moby/blob/v24.0.0-beta.2/registry/config.go#L25-L48
// See: https://github.com/moby/moby/blob/v24.0.2/registry/config.go#L25-L48

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

updated the version number.

Copy link
Contributor

@shizhMSFT shizhMSFT left a comment

Choose a reason for hiding this comment

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

LGTM

registry.go Outdated
// ServerAddressFromHostname maps a hostname to a server address, which is used as
// a key for credentials store. It is expected that the traffic targetting the
// host "registry-1.docker.io" will be redirected to "https://index.docker.io/v1/".
// https://github.com/moby/moby/blob/v24.0.2/registry/config.go#L25-L48
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: consistency

Suggested change
// https://github.com/moby/moby/blob/v24.0.2/registry/config.go#L25-L48
// See: https://github.com/moby/moby/blob/v24.0.2/registry/config.go#L25-L48

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good catch.

Copy link
Contributor

@shizhMSFT shizhMSFT left a comment

Choose a reason for hiding this comment

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

LGTM

@shizhMSFT shizhMSFT merged commit 6d14c42 into oras-project:main Jun 19, 2023
@wangxiaoxuan273 wangxiaoxuan273 deleted the expose branch June 19, 2023 09:13
@wangxiaoxuan273 wangxiaoxuan273 mentioned this pull request Jun 19, 2023
3 tasks
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Expose the registry name mapping methods
4 participants