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.

Sign up for GitHub

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

cli: inconsistent precedence for registry flag #11144

Merged
merged 1 commit into from
Jul 28, 2023
Merged

cli: inconsistent precedence for registry flag #11144

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 13 additions & 5 deletions jaeger/cmd/install.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,13 @@ func newCmdInstall() *cobra.Command {
var wait time.Duration
var options values.Options

// If LINKERD_DOCKER_REGISTRY is not null, use it as default registry path.
// If --registry option is provided, it will override the env variable.
defaultDockerRegistry := pkgcmd.DefaultDockerRegistry
if regOverride := os.Getenv(flags.EnvOverrideDockerRegistry); regOverride != "" {
defaultDockerRegistry = regOverride
}

cmd := &cobra.Command{
Use: "install [flags]",
Args: cobra.NoArgs,
Expand Down Expand Up @@ -75,7 +82,7 @@ A full list of configurable values can be found at https://www.github.com/linker
},
}

cmd.Flags().StringVar(&registry, "registry", pkgcmd.DefaultDockerRegistry,
cmd.Flags().StringVar(&registry, "registry", defaultDockerRegistry,
fmt.Sprintf("Docker registry to pull jaeger-webhook image from ($%s)", flags.EnvOverrideDockerRegistry))
cmd.Flags().BoolVar(&skipChecks, "skip-checks", false, `Skip checks for linkerd core control-plane existence`)
cmd.Flags().BoolVar(&ignoreCluster, "ignore-cluster", false,
Expand Down Expand Up @@ -151,13 +158,14 @@ func render(w io.Writer, valuesOverrides map[string]interface{}, registry string
}

regOrig := vals["webhook"].(map[string]interface{})["image"].(map[string]interface{})["name"].(string)

// registry variable can never be empty. The precedence are as:
// 1. --registry
// 2. EnvOverrideDockerRegistry
// 3. DefaultDockerRegistry
if registry != "" {
vals["webhook"].(map[string]interface{})["image"].(map[string]interface{})["name"] = pkgcmd.RegistryOverride(regOrig, registry)
Copy link
Member

Choose a reason for hiding this comment

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

It appears that the env var stopped working for the linkerd jaeger install command:

$ LINKERD_DOCKER_REGISTRY=ENV bin/go-run cli jaeger install --ignore-cluster  | grep image: | sort | uniq
        image: cr.l5d.io/linkerd/jaeger-webhook:dev-09626867-alpeb
        image: jaegertracing/all-in-one:1.31
        image: otel/opentelemetry-collector:0.59.0

The reason is registry in this line will always be non-empty, holding its default value pkgcmd.DefaultDockerRegistry.

As for the previous CI failure, it was just a flake, I re-triggered the tests and they went through 😉

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The reason is registry in this line will always be non-empty, holding its default value

Oh! I assumed that that was what we expected (I donno why I assumed it, makes no sense). I have updated the PR, can you please have a look and let me know if I need to take a different approach? Moreover, I'm kind of new to Go so, pls do let me know if I am not following any convention here.

As for the previous CI failure, it was just a flake, I re-triggered the tests and they went through 😉

Thanks a lot! I was just amused by the possibility of auto-re-triggers!

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for the quick turnaround 👍
Can you actually leave the default value for the --registry flag as it originally was? IMO those defaults should be static and independent of the environment. Maybe the same approach you had for multicluster/cmd/link.go can work here as well? Just using this at the beginning:

	if registry := os.Getenv(flags.EnvOverrideDockerRegistry); registry != "" {
		opts.dockerRegistry = registry
	}

and then inside render() leave vals["webhook"].(map[string]interface{})["image"].(map[string]interface{})["name"] = pkgcmd.RegistryOverride(regOrig, registry) regardless of registry's content...

Copy link
Contributor Author

@harsh020 harsh020 Jul 28, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Hey @alpeb, I think I'm a bit confused here. When you say don't replace default value in flag do you mean the default variable pkgcmd.DefaultDockerRegistry (or specifically the value it holds) with some other variable. Because I am doing the same thing as I have done in multicluster/cmd/link.go, here I have just introduces a new variable deafultRegistry (in multicluster we had opts). Do you want me to not use this extra variable and replace the value in pkgcmd.DefaultDockerRegistry itself (I was a bit skeptical about this as maybe it could have messed the global state).

Copy link
Member

Choose a reason for hiding this comment

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

You're right, I take that back, you're doing the same for the link command. And it would require unwarranted added complexity to do it like I suggest.

}
// env var overrides CLI flag
if override := os.Getenv(flags.EnvOverrideDockerRegistry); override != "" {
vals["webhook"].(map[string]interface{})["image"].(map[string]interface{})["name"] = pkgcmd.RegistryOverride(regOrig, override)
}

fullValues := map[string]interface{}{
"Values": vals,
Expand Down
11 changes: 7 additions & 4 deletions multicluster/cmd/link.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,13 @@ type (

func newLinkCommand() *cobra.Command {
opts, err := newLinkOptionsWithDefault()

// Override the default value with env registry path.
// If cli cmd contains --registry flag, it will override env variable.
if registry := os.Getenv(flags.EnvOverrideDockerRegistry); registry != "" {
opts.dockerRegistry = registry
}

var valuesOptions valuespkg.Options

if err != nil {
Expand Down Expand Up @@ -423,10 +430,6 @@ func buildServiceMirrorValues(opts *linkOptions) (*multicluster.Values, error) {
return nil, err
}

if reg := os.Getenv(flags.EnvOverrideDockerRegistry); reg != "" {
opts.dockerRegistry = reg
}

defaults.TargetClusterName = opts.clusterName
defaults.ServiceMirrorRetryLimit = opts.serviceMirrorRetryLimit
defaults.LogLevel = opts.logLevel
Expand Down