Skip to content

feat(api/log)!: support paging on GetBuildLogs #798

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

Merged
ecrupper merged 11 commits into from
Apr 10, 2023
Merged

feat(api/log)!: support paging on GetBuildLogs #798

Changes from 3 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
88c0df7
feat(api/log): support paging on GetBuildLogs
cognifloyd Mar 2, 2023
5e1c4b0
Merge branch 'main' into log-paging
wass3rw3rk Mar 24, 2023
9368375
Merge branch 'main' into log-paging
wass3rw3rk Mar 24, 2023
2328018
Update api/log.go
cognifloyd Mar 24, 2023
404163c
refactor(api/log): cap max per_page at 100
cognifloyd Mar 25, 2023
29e8aff
Merge branch 'main' into log-paging
wass3rw3rk Mar 27, 2023
ed5f252
enhance(api/log)!: lower default per_page to 10 from 100 (the max)
cognifloyd Mar 28, 2023
f8f03d0
Merge branch 'main' into log-paging
KellyMerrick Apr 5, 2023
3c43516
Merge branch 'main' into log-paging
wass3r Apr 6, 2023
04ba4ab
Merge branch 'main' into log-paging
cognifloyd Apr 8, 2023
9aea6af
Merge branch 'main' into log-paging
cognifloyd Apr 8, 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
47 changes: 39 additions & 8 deletions api/log.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,15 +7,15 @@ package api
import (
"fmt"
"net/http"

"github.com/go-vela/server/router/middleware/org"
"github.com/go-vela/server/router/middleware/user"
"strconv"

"github.com/go-vela/server/database"
"github.com/go-vela/server/router/middleware/build"
"github.com/go-vela/server/router/middleware/org"
"github.com/go-vela/server/router/middleware/repo"
"github.com/go-vela/server/router/middleware/service"
"github.com/go-vela/server/router/middleware/step"
"github.com/go-vela/server/router/middleware/user"
"github.com/go-vela/server/util"

"github.com/go-vela/types/library"
Expand Down Expand Up @@ -47,6 +47,17 @@ import (
// description: Build number
// required: true
// type: integer
// - in: query
// name: page
// description: The page of results to retrieve
// type: integer
// default: 1
// - in: query
// name: per_page
// description: How many results per page to return
// type: integer
// maximum: 500
// default: 100
Copy link
Member Author

@cognifloyd cognifloyd Mar 24, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doc comment is a bit clearer on my intention. @jbrockopp do you want the maximum to be 100? I would like to increase that for this endpoint.

Copy link
Contributor

@jbrockopp jbrockopp Mar 24, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, we shouldn't allow anything more than 100 results per page 👍

Especially for something like logs which can be quite taxing to retrieve depending on the size

Copy link
Member

@wass3rw3rk wass3rw3rk Mar 24, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

even 100 seems like a lot, tbh.

Copy link
Member Author

@cognifloyd cognifloyd Mar 25, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I dropped the maximum back to 100.

And I left the default at 100 so that it is backwards compatible. Does that look better?

Copy link
Contributor

@jbrockopp jbrockopp Mar 28, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cognifloyd apologies if my initial review left you under the impression that this breaking change is bad.

More so, I wanted to confirm it because if so, we try to update the PR title with a !

i.e. feat(api/log): support paging on GetBuildLogs -> feat(api/log)!: support paging on GetBuildLogs

FWIW - I'm not opposed to making the default 10 instead of current behavior of 100.

Like @wass3rw3rk mentioned, 100 does seem like a lot since it can be taxing on the system.

i.e. someone has a build with a bunch of steps that each have > 1 MB in logs

For now, I'll plan to approve this PR so if we don't want to change it, then we can consider it in the future.

Copy link
Member Author

@cognifloyd cognifloyd Mar 28, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok. I can add ! (I'm slowly learning this commit format) and switch to 10 by default. I'll do that in 30 min or so

Copy link
Member Author

@cognifloyd cognifloyd Mar 28, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. I lowered the per_page default to 10 and marked this PR as a breaking change.

Copy link
Member

@wass3rw3rk wass3rw3rk Mar 28, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks.. i like it. we can either hardcode in CLI as @jbrockopp said and/or consider dropping hints if they are viewing a subset of available data.

// security:
// - ApiKeyAuth: []
// responses:
Expand Down Expand Up @@ -82,10 +93,30 @@ func GetBuildLogs(c *gin.Context) {
"user": u.GetName(),
}).Infof("reading logs for build %s", entry)

// capture page query parameter if present
page, err := strconv.Atoi(c.DefaultQuery("page", "1"))
if err != nil {
//nolint:lll // ignore long line length due to error message
retErr := fmt.Errorf("unable to convert page query parameter for build %s: %w", entry, err)
util.HandleError(c, http.StatusBadRequest, retErr)

return
}

// capture per_page query parameter if present
perPage, err := strconv.Atoi(c.DefaultQuery("per_page", "10"))
if err != nil {
retErr := fmt.Errorf("unable to convert per_page query parameter for build %s: %w", entry, err)
util.HandleError(c, http.StatusBadRequest, retErr)

return
}

// ensure per_page isn't above or below allowed values
perPage = util.MaxInt(1, util.MinInt(500, perPage))
Copy link
Contributor

@jbrockopp jbrockopp Mar 24, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this should be 100 based off other API code that calls util.MinInt(<int>, perPage)

Copy link
Member Author

@cognifloyd cognifloyd Mar 24, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I meant to have it do the same thing as before - the comment says default 100 but I accidentally used 10. I'll fix that.

I think there can be more than 100 logs, especially if we add the InitStep logs from my proposal. So, I wanted to allow more than 100 per page, even though other endpoints allow at most 100 per page.

Do we really want the per_page max to be 100, the same as the default?

Copy link
Contributor

@jbrockopp jbrockopp Mar 24, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, we shouldn't allow anything more than 100 results per page 👍

Especially for something like logs which can be quite taxing to retrieve depending on the size

Copy link
Member

@wass3rw3rk wass3rw3rk Mar 24, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fwiw, i wasn't concerned about the 10 being the default, but you're right it maybe should have been marked as breaking.


// send API call to capture the list of logs for the build
//
// TODO: add page and per_page query parameters
l, t, err := database.FromContext(c).ListLogsForBuild(b, 1, 100)
l, t, err := database.FromContext(c).ListLogsForBuild(b, page, perPage)
if err != nil {
retErr := fmt.Errorf("unable to get logs for build %s: %w", entry, err)

Expand All @@ -96,8 +127,8 @@ func GetBuildLogs(c *gin.Context) {

// create pagination object
pagination := Pagination{
Page: 1,
PerPage: 100,
Page: page,
PerPage: perPage,
Total: t,
}
// set pagination headers
Expand Down