-
Notifications
You must be signed in to change notification settings - Fork 31
Configuration CLI Tool for Pelican #1693
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
Configuration CLI Tool for Pelican #1693
Conversation
a1c7ce4 to
5a080c6
Compare
jhiemstrawisc
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just want to say this was a really solid implementation of this tool for your first major PR -- great job! There are lots of comments and change requests, but that's inevitable when the code churn grows this large and when you haven't spent hours and hours in the code base.
A few top-level comments for you to consider (and you'll see some of these repeated throughout the review):
- We almost never want to call
fmt.Printdirectly -- unless these are the final result of a top-level CLI command. Most of these should be switched to use our logging package. - In some places you removed a
.GetName()call and replaced it with a directviper.Set("blah")-- I want to see this happening in the opposite direction, because grabbing a name from our param package turns a runtime error into a compilation error, which means we'll catch it before the user does. I don't think there's need to go back and change all of the viper sets to use this, but please look for and revert any cases where you made this switch explicitly. - I'm not in love with the fact that
config dumpprints values that are unset and empty. For example,config dumpgenerates this snippet for me right now:
Plugin:
Token: ""
There are some defaults that aren't their value type's assumed "null" value, but in cases where the variable is unset and has no value, we should omit the line entirely. Is that doable?
4. The config sum/summary command generates a nil dereference for me. I included the full debug output for you in my inline comment, but when you fix please consider what kind of unit test would have caught this bug had the test already existed.
If you disagree with any of my comments or requests, always feel free to push back and disagree, but be prepared to back up your opinion -- you are the expert with respect to these code changes, so I may be making incorrect assumptions.
jhiemstrawisc
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Getting very close, but there are still two things I'd like you to clean up:
- In
cmd/config_printer/config_printer_test.go, the call toInitConfigDir()in yoursetupMockConfig()helper function still sets/etc/pelicanas the config directory when I run the tests as root. Even though we don't actually read the config there as a part of the test, I'd like you to do aviper.Set("ConfigDir", t.TempDir())insetupMockConfigto make sure any future additions to the test are less likely to leak into my personal config. - There appears to be an issue with
config summarywhere configurable lists that are empty by default are still being printed:
$ pelican config summary
... A bunch of stuff omitted for brevity ...
Origin:
ExportVolumes: [] --> this shouldn't be printed
Server:
Modules: [] --> this shouldn't be printed
One note about these two empty slices -- in parameters.yaml, both have type stringSlice, but Origin.ExportVolumes has a default of none and Server.Modules has a default of []. Whichever of these defaults we consider to be "correct," we should be consistent. If you think cleaning that up is outside the scope of this PR, please open an issue so we can keep track of it.
|
I have created an issue for the inconsistency in empty stringSlice representation in documentation. |
jhiemstrawisc
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎉
… to prevent pipe buffer overflow
… restored the Origin.AudienceURL comment
3e8cd45 to
3ec3954
Compare
Pull Request for Issue #1427: Configuration CLI Tool for Pelican
This PR resolves #1427 by introducing a command-line interface tool to retrieve, query, and get documentation for the configuration parameters in the Pelican system.
Commands and Descriptions
configpelican configconfig dump [flags]pelican config dump -o jsonconfig get [arguments]pelican config get log monitor -m origin -m cache --include-deprecatedconfig describe [param]pelican config describe server.webPortconfig summary [flags]pelican config summary -o jsonBroad Overview of Changes Made in the Code
Embedded
docs/parameters.yamlin the BinaryThis was necessary to enable the
describecommand to output the documentation for specified configuration parameters directly from the binary.Separated Configuration Initialization Code
Refactored code in
config/config.goto decouple configuration setup from server/client initialization logic within functions likeInitServerandInitClient. This change isolates configuration initialization from side effects that occur during server or client startup, so that config values can be printed independently of program execution. This separate initialization code is now invoked when printing configuration, allowing the user to see the configuration values that would be applied if a server or client program were started.Refactored Configuration-Related Functions to Use a Viper Config Instance
Modified multiple configuration-related functions to work with a passed
viperconfiguration instance instead of operating directly on the globalviperinstance. This was essential for calculating default values by passing a newviperinstance through these functions.