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.

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

Add --version flag to pelican-client #199

Merged
merged 3 commits into from
Oct 11, 2023

Conversation

haoming29
Copy link
Contributor

@haoming29 haoming29 commented Oct 9, 2023

According to issue #150 , we want to expose --version flag for pelican-client and display the version/build info to the user in a similar behavior as stashcp did. This PR addressed this issue but in a bit hacky way under various constraints from cobra.

  • In order for all commands and subcommands to respond to a flag, we need to add logic to each individual command/subcommand in cobra. This can be done in a batch using PersistentPreRun function, to add one handler for the root command and its child subcommands. However, cobra will not execute PersistentPreRun function for the command if the Run function of the command is not defined. See issue 2039. For us, many subcommands are only a level of hierarchy and won't handle any command logic themselves, such as origin. Adding Run functions will break it.
  • One workaround is to handle the flag logic inside the main function of the command, i.e. in root.go. However, the flag value won't be evaluated until Execute() function is called, which means we can't do anything using cobra in root.go
  • This leads to the final solution this PR picked, which is to manually handle --version logic in main.go by reading the last command line argument. This means that for any command ending with --version, the version info will be popped up no matter what command/argument/flag before it was. This should be the expected behavior for the --version flag.
  • I registered the flag in cobra so that the --help command will it as a global flag to users, but cobra didn't handle any of the actual logic.
  • Also, -v shorthand was disabled because we had one reserved for origin serve where -v means the volume to attach to the origin server.

Another note for those who want to improve the test coverage for the cmd package. You might want to clear the os.Args before your test logic and restore it afterwards if you are not testing cobra but just the manually-added CLI logic. I had this issue with the test case in CI for Windows OS where the test will pass some weird arguments to my main.go (which wasn't even part of my testing code) and cause the test case fail by exiting the program with command not found error.

@haoming29 haoming29 linked an issue Oct 9, 2023 that may be closed by this pull request
@bbockelm
Copy link
Collaborator

On the Cobra side -- would it be better to take the approach here:

https://gianarb.it/blog/golang-mockmania-cli-command-with-cobra

where we invoke the cobra command object directly and provide it with a byte buffer? E.g.,

	cmd := NewRootCmd()
	b := bytes.NewBufferString("")
	cmd.SetOut(b)
	cmd.SetArgs([]string{"--in", "testisawesome"})
	cmd.Execute()
	out, err := ioutil.ReadAll(b)

This is a little less thorough than invoking the top-level function (meaning it'd potentially miss bugs). Would it simplify the unit test though?

@haoming29
Copy link
Contributor Author

On the Cobra side -- would it be better to take the approach here:

https://gianarb.it/blog/golang-mockmania-cli-command-with-cobra

where we invoke the cobra command object directly and provide it with a byte buffer? E.g.,

	cmd := NewRootCmd()
	b := bytes.NewBufferString("")
	cmd.SetOut(b)
	cmd.SetArgs([]string{"--in", "testisawesome"})
	cmd.Execute()
	out, err := ioutil.ReadAll(b)

This is a little less thorough than invoking the top-level function (meaning it'd potentially miss bugs). Would it simplify the unit test though?

Yeah I think for Cobra this would be nice way to unit test instead of invoking the main function.

Copy link
Member

@jhiemstrawisc jhiemstrawisc left a comment

Choose a reason for hiding this comment

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

As is, it looks good to me. I'll approve for now and let you decide if you want to simplify the unit test. If you decide not to change anything, go ahead and merge when you get the chance. If you make changes, I'll re-review.

@haoming29
Copy link
Contributor Author

As is, it looks good to me. I'll approve for now and let you decide if you want to simplify the unit test. If you decide not to change anything, go ahead and merge when you get the chance. If you make changes, I'll re-review.

I think for the --version flag, we can't really take advantage of the "buffer to cobra command" as the handleCLI function directly catch command line argument and I bypassed cobra completely for this. But we can save this tip for future cobra tests for sure.

@haoming29 haoming29 merged commit a45de00 into PelicanPlatform:main Oct 11, 2023
6 checks passed
@haoming29 haoming29 deleted the add-version-flag branch October 11, 2023 14:10
@bbockelm bbockelm added this to the v7.2.0 milestone Oct 12, 2023
@bbockelm bbockelm added the enhancement New feature or request label Oct 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Expose pelican version as a runnable flag
3 participants