Add ability to override default registry and tag in pkg/name #824
Conversation
Adds a name OptionFn type that allows for overriding default option values. Also introduces two new option fields, defaultRegistry and defaultTag. These are set to the previously used defaults unless overridden by their respective OptionFns. Signed-off-by: hasheddan <georgedanielmangum@gmail.com>
Moves registry name defaulting to construction rather than defaulting in registry methods. This allows for a user to override the configured default via name options. Note that we still rewrite docker.io to index.docker.io if it is explictly provided. This also means that if the default is set to docker.io by a user and no registry is provided, we will rewrite the defeault to index.docker.io. Signed-off-by: hasheddan <georgedanielmangum@gmail.com>
Moves tag defaulting to construction of the tag object rather than defaulting in tag methods. Note that we assume a default tag is valid in the eyes of the user and do not check the default for validity if it is overridden. This is consistent with the current behavior as defaulting was previously happening much later than the check for a valid tag. Signed-off-by: hasheddan <georgedanielmangum@gmail.com>
This is interesting. Do you have a use case for this in mind, or are you doing this for consistency sake? |
|
@jonjohnsonjr some of both. For instance, in @crossplane we don't use the |
|
I had also briefly toyed with the |
| func TestOverrideDefaultRegistryNames(t *testing.T) { | ||
| testRegistries := []string{"docker.io", ""} | ||
| expectedRegistries := []string{"index.docker.io", "gcr.io"} | ||
| overrideDefault := "gcr.io" |
There was a problem hiding this comment.
What happens if this is gcr.io/myproject?
There was a problem hiding this comment.
It would use gcr.io/myproject. We do not check the default, so a user has the ability to provide something nonsensical there. We were not previously validating the default either, but there was also only one option that could ever be the default. I could go either way on this, but I lean towards letting the user do what they want if they are opting to override. Typically I imagine the override will be hardcoded, rather than changing on accepted input, so the package consumer is responsible for making sure they use a reasonable value. What do you think?
There was a problem hiding this comment.
I could go either way on this, but I lean towards letting the user do what they want if they are opting to override.
Yeah there is precedent for this (repo.Tag(), repo.Digest()), so I think it's fine.
I'd like to see tests for this that actually calls ParseReference instead of NewRegistry (basically nobody does that). A list of triples with (input, defaultRegistry, output) that includes this case would be great.
There was a problem hiding this comment.
@jonjohnsonjr SGTM. Would you be open to these tests being refactored to table-driven in a follow-up PR?
There was a problem hiding this comment.
@jonjohnsonjr I have updated with the ParsseReference tests here 👍
There was a problem hiding this comment.
Would you be open to these tests being refactored to table-driven in a follow-up PR?
Yeah that's fine, honestly this whole package needs that treatment and some docs love.
There was a problem hiding this comment.
@jonjohnsonjr sounds good! I'll get to work on that next 👍 Any more updates needed here?
Adds tests for paring full image references with registry and tag defaulting. Signed-off-by: hasheddan <georgedanielmangum@gmail.com>
Codecov Report
@@ Coverage Diff @@
## master #824 +/- ##
==========================================
+ Coverage 74.63% 74.66% +0.03%
==========================================
Files 103 103
Lines 4340 4346 +6
==========================================
+ Hits 3239 3245 +6
Misses 619 619
Partials 482 482
Continue to review full report at Codecov.
|
Adds default registry and default tag options to
pkg/namesuch that a user may configure their own defaults. Also move defaulting logic to tag / registry construction rather than methods called on the objects. See individual commits for how this maintains compatibility with existing behavior./cc @jonjohnsonjr @imjasonh
Fixes #805