-
-
Notifications
You must be signed in to change notification settings - Fork 100
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 flags for handling features of advanced logs #471
Conversation
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.
Need to add the completions as I suggested below.
Additionally I would strongly suggest refactoring this into two helpers: one for adding the flags and one for processing them. This same code is copied and pasted in 7 places it looks like, we should use common helpers instead.
Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍 |
Thanks for the review, @mdegat01, it looks indeed better this way. There was some specific error handling implemented only for addons, not sure why. I have removed it, but if we want to keep it, I don't see a reason why only one of the endpoints should get special handling. Also added the completions, check the individual commits for less diff noise. |
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.
This looks good.
I guess my minor organization suggestion would be I think it makes more sense to have addLogsFlags
and processLogsFlags
in cmd/host.go
rather then cmd/root.go
. I would also probably put hostBootCompletions
in there for good measure. But not necessary, it all works fine like this.
Add flags for features previously available only to host logs.