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

feat: expose registry name mapping methods#76

Merged
shizhMSFT merged 1 commit into
oras-project:mainfrom
wangxiaoxuan273:expose
Jun 19, 2023
Merged

feat: expose registry name mapping methods#76
shizhMSFT merged 1 commit into
oras-project:mainfrom
wangxiaoxuan273:expose

Conversation

@wangxiaoxuan273

Copy link
Copy Markdown
Collaborator

Resolves #72

@codecov-commenter

codecov-commenter commented Jun 12, 2023

Copy link
Copy Markdown

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%) ⬇️

@TerryHowe TerryHowe left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

/lgtm

Comment thread registry.go Outdated
}

func mapStoreRegistryName(registry string) string {
func MapRegistryNameDockerIo(registry string) string {

Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

changed accordingly.

Comment thread registry.go Outdated
}

func mapAuthenticationRegistryName(hostname string) string {
func MapRegistryNameRegistry1DockerIo(hostname string) string {

Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

changed accordingly.

Comment thread 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
Copy Markdown
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
Copy Markdown
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.

@shizhMSFT shizhMSFT left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

Comment thread 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
Copy Markdown
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
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Good catch.

Signed-off-by: Xiaoxuan Wang <wangxiaoxuan119@gmail.com>

@shizhMSFT shizhMSFT left a comment

Copy link
Copy Markdown
Contributor

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