Add ParseTagOption to k8s/go/pkg/resolver #698
Conversation
|
Hi @gonzojive. Thanks for your PR. I'm waiting for a bazelbuild member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
/test all |
|
Can you please get this to pass tests? |
Done. |
|
/ok-to-test |
| parseTag func(name string, opts ...name.Option) (name.Tag, error) | ||
| } | ||
|
|
||
| // ParseTagOption specifies a function to be used instead of name.NewTag to parse image tags. |
There was a problem hiding this comment.
Is there a reason to add a hook here vs just replacing the resolver binary?
Line 284 in 6323c19
There was a problem hiding this comment.
Yes, this is needed to override the name resolution heuristics. Please see the example program below. A custom binary is still needed. However, the binary can reuse all the work done in the resolver library.
| @@ -58,14 +58,41 @@ func RegisterFlags(flagset *flag.FlagSet) *Flags { | |||
| return &flags | |||
| } | |||
|
|
|||
| // Option can be passed to NewResolver to configure the resolver. | |||
| type Option struct { | |||
There was a problem hiding this comment.
I'm a bit confused about why we're exporting the struct to other packages but not the methods?
Can you show me a concrete use case for how you want to use this?
There was a problem hiding this comment.
The Option pattern is pretty common in Go to parameterize a function with optional arguments.
By using your library within a custom resolver binary, it's possible to replace the heuristics built into the resolver library with custom logic without needing to fork the library.
var (
insecureRegistries = map[string]struct{}{
"localhost:32000": {},
"othermachine:32000": {},
}
)
func main() {
flagset := flag.NewFlagSet(os.Args[0], flag.ExitOnError)
flags := lib.RegisterFlags(flagset)
flagset.Parse(os.Args[1:])
resolver := lib.NewResolver(flags, lib.ParseTagOption(parseImageTag))
resolved, err := resolver.Resolve()
if err != nil {
log.Fatal(err)
}
fmt.Println(resolved)
}
func parseImageTag(t string, opts ...name.Option) (name.Tag, error) {
got, err := name.NewTag(t, opts...)
if err != nil {
return got, err
}
regName := got.Context().Registry.Name()
if _, isInsecure := insecureRegistries[regName]; isInsecure {
var allOpts []name.Option
allOpts = append(allOpts, opts...)
allOpts = append(allOpts, name.Insecure)
return name.NewTag(t, allOpts...)
}
return got, fmt.Errorf("unknown registry name %q", regName)
}There was a problem hiding this comment.
I don't see how that is possible when Options struct rather than an interface. No one will be able to provide an alternative implementation of apply() (which by being lower case is also inaccessible to other packages)
There was a problem hiding this comment.
The resolver library provides the only means of constructing an Option, like ParseTagOption. I don't see a need for any other option types at a this time, so I kept the type private to the package. You can always open it up later.
This pattern is used by the cmp library, though it uses an interface with unexported methods rather than a struct. Either works. https://pkg.go.dev/github.com/google/go-cmp/cmp#Option
There was a problem hiding this comment.
I'm not a huge fan of this API design, but I guess we can change it later if necessary.
|
PS: mostly trying to better understand the need here (aka make it easier to configure if an image is secure) and why this is a good approach for addressing that need. |
| @@ -58,14 +58,41 @@ func RegisterFlags(flagset *flag.FlagSet) *Flags { | |||
| return &flags | |||
| } | |||
|
|
|||
| // Option can be passed to NewResolver to configure the resolver. | |||
| type Option struct { | |||
There was a problem hiding this comment.
I'm not a huge fan of this API design, but I guess we can change it later if necessary.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: fejta, gonzojive The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
No description provided.