-
Notifications
You must be signed in to change notification settings - Fork 8
feat(logging): add dynamic logging CLI and management API #173
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
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.
Good stuff! Thank you @bomanaps <3
Sorry for the churn here, but I think it's best if we deviate from the initial issue - as will be evident from a few of the comments. Let me know if you have any questions.
cmd/cli/root.go
Outdated
| rootCmd.PersistentFlags().String("manage-api", "127.0.0.1:8888", "Management API address") | ||
| cobra.CheckErr(viper.BindPFlag("manage_api", rootCmd.PersistentFlags().Lookup("manage-api"))) | ||
|
|
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.
Per https://github.com/storacha/piri/pull/173/files#r2257733914 we can remove this.
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.
Be sure to remove this still
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.
looking good, few more comments
cmd/cli/root.go
Outdated
| rootCmd.PersistentFlags().String("manage-api", "127.0.0.1:8888", "Management API address") | ||
| cobra.CheckErr(viper.BindPFlag("manage_api", rootCmd.PersistentFlags().Lookup("manage-api"))) | ||
|
|
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.
Be sure to remove this still
|
Hello @frrist I added this unit test to it TestLogList_PrintsSubsystemsAndLevels: Mock server returns subsystems and levels, verify formatted output. |
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.
Almost there, few more comments to address then we should be good to merge. thanks for the hard work here 🙂
|
Hello @frrist please am still waiting for your final comment on this? |
|
Hey @frrist please can you trigger the ci again although am not sure how to go about the AWS fail |
|
Thanks @bomanaps will get this merged before our next release. Appreciate the effort you put in here ❤️. |
This Pr closes this #97
I have:
log levels.
The log command can be used like this:
subsystem.
Please Note this is still a WIP and I still have some missing uint test implementation