-
Notifications
You must be signed in to change notification settings - Fork 55
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
DXCDT-403: Separate rendering for logs tail and logs list #672
Conversation
@@ -144,7 +144,7 @@ func (v *logView) typeDesc() (typ, desc string) { | |||
return typ, desc | |||
} | |||
|
|||
func (r *Renderer) LogList(logs []*management.Log, ch <-chan []*management.Log, silent bool) { | |||
func (r *Renderer) LogList(logs []*management.Log, silent bool) { |
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.
Now that we're separating them, if we run auth0 logs list
with a filter and we get back no log messages of that type we'll post a message that "No logs are available.", although that's slightly incorrect.
There probably are log messages just not any that satistfy the filter. So maybe we can improve the output with something like:
auth0 logs list -n 10 --filter "type:f"
=== tenant.example.auth0.com logs
No logs available matching filter criteria.
▸ To generate logs, run a test command like 'auth0 test login' or 'auth0 test token'
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.
The behavior you're describing is not caused by this change, it existed prior. Though I do agree with the general sentiment.
Implementing isn't exactly trivial because the render functions are abstracted away from knowing the filters and other arguments. The best we could do here is to provide a generalized message that would apply to alls situations regardless of provided criteria.
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.
The behavior you're describing is not caused by this change, it existed prior. Though I do agree with the general sentiment.
Well we can still leave the codebase a bit better than it was, and also improve DX considering we're now touching this part, but I'm also okay if we wanna do this in a separate PR.
To implement this we can check inside listLogsCmd()
if we have any filters selected and pass it to the LogList()
func.
hasFilter := inputs.Filter != ""
cli.renderer.LogList(list, !cli.debug, hasFilter)
then inside the LogList()
we can tweak the if len(logs) == 0
check as follows:
if len(logs) == 0 {
if hasFilter {
r.Output("No logs available matching filter criteria.\n\n")
} else {
r.EmptyState(resource)
}
r.Infof("To generate logs, run a test command like 'auth0 test login' or 'auth0 test token'")
return
}
This will have the desired behavior as described above.
go run cmd/auth0/main.go logs ls -f "type:f"
=== example.auth0.com logs
No logs available matching filter criteria.
▸ To generate logs, run a test command like 'auth0 test login' or 'auth0 test token'
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.
Let's address in a follow-up PR that's more focused.
r.Results(res) | ||
} | ||
|
||
func (r *Renderer) LogTail(logs []*management.Log, ch <-chan []*management.Log, silent bool) { |
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.
The LogTail doesn't seem to attach at the top the table headers like the auth0 logs list does:
TYPE DESCRIPTION DATE CONNECTION CLIENT
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #672 +/- ##
==========================================
- Coverage 54.50% 54.46% -0.05%
==========================================
Files 89 89
Lines 11509 11512 +3
==========================================
- Hits 6273 6270 -3
- Misses 4789 4796 +7
+ Partials 447 446 -1
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report in Codecov by Sentry. |
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.
Tested locally and looks good, approving as there are no new changes in behaviour and existing items have been captured for future improvements
@sergiught I've made note of the valid points that you raised. Given the current emphasis on testing, I'm going to opt to address these in a separate, focused PR. |
🔧 Changes
Separating the
LogList
rendering function intoLogTail
andLogList
. The original function served two commands –auth0 logs tail
andauth0 logs list
. Despite the similarities between these commands, their uses differed enough to warrant the separation.Specific differences include:
🔬 Testing
Primarily manual. Log tail streaming output cannot be tested with current framework though
auth0 logs list
will be covered.📝 Checklist