Skip to content
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.

Sign up for GitHub

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

Merged
merged 20 commits into from
Mar 30, 2023
Merged

DXCDT-403: Separate rendering for logs tail and logs list #672

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
fb73785
Merge branch 'main' of https://github.com/auth0/auth0-cli
willvedd Dec 5, 2022
da6a932
Merge branch 'main' of https://github.com/auth0/auth0-cli
willvedd Dec 5, 2022
0d62b4c
Merge branch 'main' of https://github.com/auth0/auth0-cli
willvedd Dec 6, 2022
a4aed9e
Merge branch 'main' of https://github.com/auth0/auth0-cli
willvedd Dec 9, 2022
fc50ba9
Merge branch 'main' of https://github.com/auth0/auth0-cli
willvedd Jan 5, 2023
054ab9e
Merge branch 'main' of https://github.com/auth0/auth0-cli
willvedd Jan 19, 2023
1aa695c
Merge branch 'main' of https://github.com/auth0/auth0-cli
willvedd Jan 20, 2023
da03c9e
Merge branch 'main' of https://github.com/auth0/auth0-cli
willvedd Jan 20, 2023
358f05c
Merge branch 'main' of https://github.com/auth0/auth0-cli
willvedd Jan 25, 2023
edb118a
Merge branch 'main' of https://github.com/auth0/auth0-cli
willvedd Feb 8, 2023
eaba3cc
Merge branch 'main' of https://github.com/auth0/auth0-cli
willvedd Feb 10, 2023
d28a33f
Merge branch 'main' of https://github.com/auth0/auth0-cli
willvedd Mar 6, 2023
6496148
Merge branch 'main' of https://github.com/auth0/auth0-cli
willvedd Mar 7, 2023
f945c87
Merge branch 'main' of https://github.com/auth0/auth0-cli
willvedd Mar 13, 2023
c749530
Separating view for log tail and log list
willvedd Mar 13, 2023
0d0fe16
Merge branch 'main' into DXCDT-403-logs-tail-no-results-view
Widcket Mar 16, 2023
04085e3
Merge branch 'main' into DXCDT-403-logs-tail-no-results-view
Widcket Mar 16, 2023
040c74b
Merge branch 'main' into DXCDT-403-logs-tail-no-results-view
willvedd Mar 27, 2023
6ad8cb6
Merge branch 'main' into DXCDT-403-logs-tail-no-results-view
willvedd Mar 29, 2023
e60804d
Merge branch 'main' into DXCDT-403-logs-tail-no-results-view
willvedd Mar 30, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 2 additions & 3 deletions internal/cli/logs.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,8 +66,7 @@ func listLogsCmd(cli *cli) *cobra.Command {
return fmt.Errorf("An unexpected error occurred while getting logs: %v", err)
}

var logsCh chan []*management.Log
cli.renderer.LogList(list, logsCh, !cli.debug)
cli.renderer.LogList(list, !cli.debug)
return nil
},
}
Expand Down Expand Up @@ -154,7 +153,7 @@ func tailLogsCmd(cli *cli) *cobra.Command {
}
}()

cli.renderer.LogList(list, logsCh, !cli.debug)
cli.renderer.LogTail(list, logsCh, !cli.debug)
return nil
},
}
Expand Down
35 changes: 20 additions & 15 deletions internal/display/logs.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -142,7 +142,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) {
Copy link
Contributor

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'

Copy link
Contributor Author

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.

Copy link
Contributor

@sergiught sergiught Mar 13, 2023 •
edited
Loading

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'

Copy link
Contributor Author

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.

resource := "logs"

r.Heading(resource)
Expand All @@ -158,23 +158,28 @@ func (r *Renderer) LogList(logs []*management.Log, ch <-chan []*management.Log,
res = append(res, &logView{Log: l, silent: silent, raw: l})
}

var viewChan chan View
r.Results(res)
}

func (r *Renderer) LogTail(logs []*management.Log, ch <-chan []*management.Log, silent bool) {
Copy link
Contributor

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

r.Heading("logs")

var res []View
for _, l := range logs {
res = append(res, &logView{Log: l, silent: silent, raw: l})
}

if ch != nil {
viewChan = make(chan View)
viewChan := make(chan View)

go func() {
defer close(viewChan)
go func() {
defer close(viewChan)

for list := range ch {
for _, l := range list {
viewChan <- &logView{Log: l, silent: silent, raw: l}
}
for list := range ch {
for _, l := range list {
viewChan <- &logView{Log: l, silent: silent, raw: l}
}
}()

r.Stream(res, viewChan) // streams results for `auth0 logs tail`
}
}
}()

r.Results(res) // Includes headers for `auth0 logs list`
r.Stream(res, viewChan)
}