cli: inconsistent precedence for registry flag

[harsh020]

Fixes: #11115

Problem:
Commands jaeger install, multicluster link give precedence to LINKERD_DOCKER_REGISTRY env var, whereas commands install, upgrade and inject give preference to --register flag.

Solution:
Make the commands consitent by giving precedence to --register flag in all commands.

Tasks:

• Make jaeger install give precedence to --register
• Make multicluster link give precedence to --register

[harsh020]

@risingspiral @alpeb I have a few questions before I proceed,

1. As the env variable is named EnvOverrideDockerRegistry it seems kind of unconventional that the --register flag would override the override. Should we rename the env variable or keep it as it is?
2. Do the changes till now (I know they are small) seem good and am I in the right direction?
3. I have not committed with the complete commit message as of now (no DCO and Validation). Do we follow force-push commits to override the commit messages here at linkered?
4. Should I pull master and resolve the conflicts in next commit or conflicts are resolved when the maintainers queue the PRs to be merged?

[harsh020]

@risingspiral @alpeb Can you please review and let me know if something else need to be done here.

[adleong]

Hi @harsh020

Please take a look at the CI failure on this PR. It look like you may be missing a cmd import.

[harsh020]

@adleong Oh my! I think I changed pkgcmd to cmd by mistake.

• But this time I did run test for jaeger/cmd and multicluster/cmd, and they seemed to be ok.
• I also ran linter on both but it seems to be failing on import statements (I assume that not because of me)

[harsh020]

@adleong Hey, the multicluster install test just cancelled due to

Waiting for control plane to become available
Cannot find Linkerd: No running pods for "linkerd-destination"

I have made no changes in the install cmd. Please correct me if I'm wrong but the error does not seem to be because of my changes, right?

EDIT: Wait what just happened, suddenly the CI that was cancelled automatically passed! Am I missing something or the workflow was retriggered?

[adleong]

Looks great!

[alpeb]

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 😉

[harsh020]

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!

[alpeb]

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...

[harsh020]

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).

[alpeb]

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.

[alpeb]

Good job!