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

use local ConvertToHostname() implementation #3599

Merged
merged 1 commit into from
Jun 20, 2024

Conversation

thaJeztah
Copy link
Member

Commit 27b2797 (#1642) added a local implementation of this function, so let's use the local variant to (slightly) reduce the dependency on moby's registry package.

Also made some minor cleanups.

- A picture of a cute animal (not mandatory but encouraged)

@thaJeztah thaJeztah added status/2-code-review kind/refactor PR's that refactor, or clean-up code labels May 10, 2022
@codecov-commenter
Copy link

codecov-commenter commented May 10, 2022

Codecov Report

Attention: Patch coverage is 87.50000% with 1 line in your changes missing coverage. Please review.

Project coverage is 61.78%. Comparing base (925e7d6) to head (8376b3e).

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3599      +/-   ##
==========================================
+ Coverage   61.76%   61.78%   +0.01%     
==========================================
  Files         297      297              
  Lines       20768    20768              
==========================================
+ Hits        12828    12831       +3     
+ Misses       7024     7023       -1     
+ Partials      916      914       -2     

@thaJeztah
Copy link
Member Author

@rumpl @ndeloof ptal 🤗

@ndeloof
Copy link
Contributor

ndeloof commented May 11, 2022

I don't think "Copied from github.com/docker/docker/registry.ConvertToHostname to reduce dependencies" makes our codebase better.

  1. we still (obviously) depend on docker/docker as the API is not an isolated project vs engine backend
  2. we now have two implementations of the same ConvertToHostname function to maintain (for sure, low maintenance expected here)

imho it would make more sense for such utility func to be shared between cli and engine within one of the api package. I'm not sure about the initial intent in #1642

@thaJeztah thaJeztah force-pushed the use_local_ConvertToHostname branch from ab2bdbc to 713297d Compare June 13, 2024 22:05
Commit 27b2797 added a local implementation
of this function, so let's use the local variant to (slightly) reduce the
dependency on moby's registry package.

Also made some minor cleanups.

Signed-off-by: Sebastiaan van Stijn <[email protected]>
@thaJeztah thaJeztah force-pushed the use_local_ConvertToHostname branch from 713297d to 8376b3e Compare June 13, 2024 22:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/refactor PR's that refactor, or clean-up code status/2-code-review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants