-
Notifications
You must be signed in to change notification settings - Fork 8
feat: add support for displaying power status #58
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
base: main
Are you sure you want to change the base?
Conversation
|
@synackd Would you mine reviewing this one, I seem to have lost my powers to add a reviewer ... |
synackd
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.
Thanks for the contribution! Can you provide testing instructions? My PCS environment is wonky, but I'd like to test if I can.
cmd/pcs.go
Outdated
| // xnames holds the list of xnames provided via command line flag | ||
| var xnames []string | ||
|
|
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.
Can we put a PCS prefix on this variable? Since this is in the cmd package, this would be a variable global to all commands.
On that note, we probably need to refactor the commands to be in their own packages to avoid this issue subsequently...
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.
Consequently, any package-level variables for PCS should have a prefix for now.
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.
Yes, I agree each command should have is own package. I moved the definition of the variables into their respective files, no real need to reuse the same variable and this makes usage more clear.
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 see you've added the command prefix (e.g. transitionXnames) but could we also prepend pcs to distinguish that this is part of the pcs transition command? E.g. pcsTransitionXnames
Signed-off-by: Chris Harris <cjh@lbl.gov>
Pull Request Template
Thank you for your contribution! Please ensure the following before submitting:
Checklist
make test(or equivalent) locally and all tests passgit commit -s) with my real name and email<filename>.licensesidecarLICENSES/directoryDescription
Add support for showing/listing the power status of components
Type of Change
For more info, see Contributing Guidelines.