Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

--platform needs to be the first flag on the commandline, otherwise it is ignored #1488

Closed
hash-d opened this issue May 23, 2024 · 5 comments

Comments

@hash-d
Copy link
Member

hash-d commented May 23, 2024

Describe the bug

If a different flag precedes --platform on the command line, the value given to it is ignored.

How To Reproduce

Try a skupper init with --platform not as the first flag:

$ skupper init --ingress-host localhost --platform podman --enable-console --enable-flow-collector --site-name skgw --console-auth=internal --console-user=admin --console-password=password
Secret already exists:  skupper-local-server
Secret already exists:  skupper-local-client
Secret already exists:  skupper-service-client
Secret already exists:  skupper-console-users
Secret already exists:  skupper-site-server
Secret already exists:  skupper-console-certs
Waiting for status...
Skupper is now installed in namespace 'dh-1248'.  Use 'skupper status' to get more information.

Notice that the command above installed skupper on the K8S namespace dh-1248, instead of the requested podman install.

See that changing the placement of the --platform flag causes the correct behavior to take place.

$ skupper init --platform podman --ingress-host localhost --enable-console --enable-flow-collector --site-name skgw --console-auth=internal --console-user=admin --console-password=password
Skupper is now installed for user 'dhashimo'.  Use 'skupper status' to get more information.

Also, note that the value of the --platform is completely ignored:

$ skupper init --ingress-host localhost --platform asdf --enable-console --enable-flow-collector --site-name skgw --console-auth=internal --console-user=admin --console-password=password
Secret already exists:  skupper-local-server
Secret already exists:  skupper-local-client
Secret already exists:  skupper-service-client
Secret already exists:  skupper-console-users
Secret already exists:  skupper-site-server
Secret already exists:  skupper-console-certs
Waiting for status...
Skupper is now installed in namespace 'dh-1248'.  Use 'skupper status' to get more information.

On the example above, the invalid value asdf was given to --platform and yet it was allowed to continue.

Expected behavior

Either:

  • The value of --platform should be respected, regardless of its position on the command line, or
  • The need for --platform flag to be the first in the command line should be documented

I did not see anything on the documentation pointing to the necessity of --platform to be the first in the command line. Even in that case, an error should be generated when that's not in its proper place, instead of continuing with a configuration different from what the user specified.

Environment details

  • Skupper CLI: 1.5.3
  • Skupper Operator (if applicable): N/A
  • Platform: Minkube

Additional context

N/A

@hash-d
Copy link
Member Author

hash-d commented May 23, 2024

I've done some quick testing, and it seems the other flags, placed before or after --platform are unaffected (ie, their presence and values are respected).

@hash-d
Copy link
Member Author

hash-d commented May 23, 2024

My guess is that this is occurring because of the unchecked rootCmd.ParseFlags at the start of the init() function on cmd/skupper/skupper.go, before the actual flags are defined later on that function.

I think that call is failing because of the unknown (at the time of parsing) first option passed to the command line. Cobra probably even parses --platform correctly later on the execution, but only after the call to config.GetPlatform() that is used to select the SkuppeClient has already occurred.

@hash-d
Copy link
Member Author

hash-d commented May 23, 2024

@fgiorgetti FYI

@hash-d
Copy link
Member Author

hash-d commented May 23, 2024

Well... After opening the ticket I checked the status on main, and this is actually a duplicate of #1147, already fixed on #1451

I had not seen those before, as I searched for --platform instead of just platform.

@hash-d
Copy link
Member Author

hash-d commented May 23, 2024

Closing this as a dup

@hash-d hash-d closed this as not planned Won't fix, can't repro, duplicate, stale May 23, 2024
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant