cli/registry/client: skip RepositoryInfo as intermediate#5959
cli/registry/client: skip RepositoryInfo as intermediate#5959thaJeztah merged 1 commit intodocker:masterfrom
Conversation
thaJeztah
added
status/2-code-review
kind/refactor
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #5959 +/- ##
==========================================
- Coverage 59.12% 59.10% -0.02%
==========================================
Files 354 354
Lines 29747 29757 +10
==========================================
Hits 17587 17587
- Misses 11186 11196 +10
Partials 974 974 🚀 New features to boost your workflow:
|
thaJeztah
force-pushed
the
registry_client_skip_RepositoryInfo
branch
from
7c502f6 to
5263177
Compare
| repoName := reference.TrimNamed(ref) | ||
| repoInfo, _ := registry.ParseRepositoryInfo(ref) | ||
| endpoint, err := getDefaultEndpointFromRepoInfo(repoInfo) | ||
| indexInfo := repoInfo.Index |
There was a problem hiding this comment.
In case you're wondering why the extra variables; some of these are as a placeholder / intermediate state while I'm cleaning up things elsewhere.
thaJeztah
force-pushed
the
registry_client_skip_RepositoryInfo
branch
2 times, most recently
from
719e5c6 to
bb31ce8
Compare
| } | ||
|
|
||
| func getDefaultEndpointFromRepoInfo(repoInfo *registry.RepositoryInfo) (registry.APIEndpoint, error) { | ||
| func getDefaultEndpoint(repoName reference.Named, insecure bool) (registry.APIEndpoint, error) { |
There was a problem hiding this comment.
why are we inversing the bool here? insecure vs secure.
Since the initial property is indexInfo.Secure perhaps we should just keep it as secure bool here?
There there is no need to have
if !insecure {
...
}There was a problem hiding this comment.
Since the previous implementation would be
repoInfo.Index.Secure = true
...
if !repoInfo.Index.Secure {} // test to see if not secureBut now it is
indexInfo.Secure = true
...
getDefaultEndpoint(ref, !indexInfo.Secure) // pass along if ( not ) secure = false
...
if !insecure {} // is insecure == false? yes let's proceedThere was a problem hiding this comment.
Ugh, yes, I screwed up there and the !insecure is flipped; I think a bad rebase (I extracted this from some branches where I made other changes).
thaJeztah
force-pushed
the
registry_client_skip_RepositoryInfo
branch
from
bb31ce8 to
92345e6
Compare
| endpoint, err := getDefaultEndpointFromRepoInfo(repoInfo) | ||
| indexInfo := repoInfo.Index | ||
|
|
||
| endpoint, err := getDefaultEndpoint(ref, !indexInfo.Secure) |
There was a problem hiding this comment.
I recall now that I was looking whether this one should just get insecure (the argument) passed, which is set using the --insecure flag on docker manifest inspect, but , while related, they are technically different;
indexInfo.Secureis based on whether the registry is on a loopback address--insecureallows overriding a secure registry to be accessed insecurely (which, if the registry is using TLS allows for an invalid TLS connection).
It's really a bit of a kludge altogether 💩 🔥 (it was added as a workaround because the CLI doesn't know about the daemon configuration, which has the "insecure-registries" option).
Remove RepositoryInfo as intermediate struct in some places; we want to remove the use of this additional abstration. More changes are needed to fully remove it, but chipping away its use in small bits. Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
thaJeztah
force-pushed
the
registry_client_skip_RepositoryInfo
branch
from
92345e6 to
491e8fd
Compare
| if insecure { | ||
| for _, ep := range endpoints { | ||
| if ep.URL.Scheme == "http" { | ||
| endpoint = ep |
There was a problem hiding this comment.
This should probably also have a break here 🤔 - let's keep that for a follow-up if we think it's important.
4 participants
Remove RepositoryInfo as intermediate struct in some places; we want to remove the use of this additional abstration. More changes are needed to fully remove it, but chipping away its use in small bits.
- Human readable description for the release notes
- A picture of a cute animal (not mandatory but encouraged)