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

cognifloyd · 2023-03-24T02:44:59Z

Adds support for the page and per_page query parameters for GetBuildLogs (endpoint /api/v1/repos/+/+/builds/+/logs).

BREAKING CHANGE: per_page default lowered to 10. Before this change it always returned 1 page with up to 100 logs. To maintain the same behavior, you must now use ?per_page=100.

This resolves an in-code TODO by copying the implementation from some of the other endpoints that already implement this.

I suspect that we're going to have quite a few more Log instances to iterate over if we implement something like go-vela/community#771, so I figured I'd go ahead and resolve this while I was looking at this code.

Adds support for the page and per_page query parameters for GetBuildLogs /api/v1/repos/+/+/builds/+/logs This resolves an in-code TODO.

codecov · 2023-03-24T02:50:04Z

Codecov Report

Merging #798 (9aea6af) into main (b5777e5) will decrease coverage by 0.06%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##             main     #798      +/-   ##
==========================================
- Coverage   58.20%   58.14%   -0.06%     
==========================================
  Files         242      242              
  Lines       15721    15737      +16     
==========================================
  Hits         9150     9150              
- Misses       6165     6181      +16     
  Partials      406      406

Impacted Files	Coverage Δ
api/log.go	`0.00% <0.00%> (ø)`

wass3rw3rk

lgtm

jbrockopp

Can you confirm if this is a breaking change?

I believe it is since the behavior will default to using a per_page of 10 instead of 100.

This would impact users running vela get log via the CLI with more than 10 steps/services in their pipeline since they'd only see logs for 10 steps/services rather than 100.

jbrockopp · 2023-03-24T18:29:01Z

api/log.go

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


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

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?

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

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

api/log.go

cognifloyd · 2023-03-24T18:47:40Z

api/log.go

+// - in: query
+//   name: per_page
+//   description: How many results per page to return
+//   type: integer
+//   maximum: 500
+//   default: 100


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.

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

even 100 seems like a lot, tbh.

I dropped the maximum back to 100.

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

@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.

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

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

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.

jbrockopp

LGTM

jbrockopp

LGTM

jbrockopp · 2023-03-28T16:30:05Z

FWIW - we can always hardcode the per_page on the CLI side to 100 for vela get log to reduce the impact

Added to server in go-vela/server#798

Added to server in go-vela/server#798 Added to sdk-go in go-vela/sdk-go#216

cognifloyd · 2023-03-28T17:36:38Z

Follow-up PRs to review/merge once this one is merged:

feat(api/log): support paging on GetBuildLogs

88c0df7

Adds support for the page and per_page query parameters for GetBuildLogs /api/v1/repos/+/+/builds/+/logs This resolves an in-code TODO.

cognifloyd added the feature Indicates a new feature label Mar 24, 2023

cognifloyd self-assigned this Mar 24, 2023

cognifloyd requested a review from a team as a code owner March 24, 2023 02:45

cognifloyd changed the title ~~feat(api/log): support paging on GetBuildLogs~~ feat(api/log): support paging on GetBuildLogs Mar 24, 2023

Merge branch 'main' into log-paging

5e1c4b0

wass3rw3rk previously approved these changes Mar 24, 2023

View reviewed changes

Merge branch 'main' into log-paging

9368375

jbrockopp requested changes Mar 24, 2023

View reviewed changes

cognifloyd commented Mar 24, 2023

View reviewed changes

api/log.go Show resolved Hide resolved

cognifloyd dismissed wass3rw3rk’s stale review via 2328018 March 24, 2023 18:44

Update api/log.go

2328018

cognifloyd requested review from a team, jbrockopp and wass3rw3rk and removed request for wass3rw3rk March 24, 2023 18:45

cognifloyd commented Mar 24, 2023

View reviewed changes

refactor(api/log): cap max per_page at 100

404163c

cognifloyd requested a review from wass3rw3rk March 25, 2023 06:41

Merge branch 'main' into log-paging

29e8aff

wass3rw3rk previously approved these changes Mar 27, 2023

View reviewed changes

jbrockopp previously approved these changes Mar 28, 2023

View reviewed changes

plyr4 previously approved these changes Mar 28, 2023

View reviewed changes

enhance(api/log)!: lower default per_page to 10 from 100 (the max)

ed5f252

cognifloyd dismissed stale reviews from plyr4, jbrockopp, and wass3rw3rk via ed5f252 March 28, 2023 15:45

cognifloyd changed the title ~~feat(api/log): support paging on GetBuildLogs~~ feat(api/log)!: support paging on GetBuildLogs Mar 28, 2023

cognifloyd requested a review from jbrockopp March 28, 2023 15:49

cognifloyd requested review from a team, plyr4 and wass3rw3rk March 28, 2023 15:49

jbrockopp approved these changes Mar 28, 2023

View reviewed changes

cognifloyd added a commit to go-vela/sdk-go that referenced this pull request Mar 28, 2023

feat(build): Support paging on BuildService.GetLogs

08e24f2

Added to server in go-vela/server#798

cognifloyd mentioned this pull request Mar 28, 2023

feat(build)!: Support paging on BuildService.GetLogs go-vela/sdk-go#216

Merged

cognifloyd added a commit to go-vela/cli that referenced this pull request Mar 28, 2023

feat(log): Support paging on vela get log

5c12916

Added to server in go-vela/server#798 Added to sdk-go in go-vela/sdk-go#216

cognifloyd mentioned this pull request Mar 28, 2023

feat(log): Support paging on vela get log go-vela/cli#427

Closed

wass3rw3rk approved these changes Mar 28, 2023

View reviewed changes

KellyMerrick and others added 4 commits April 5, 2023 09:02

Merge branch 'main' into log-paging

f8f03d0

Merge branch 'main' into log-paging

3c43516

Merge branch 'main' into log-paging

04ba4ab

Merge branch 'main' into log-paging

9aea6af

wass3r mentioned this pull request Apr 8, 2023

feat(worker): add ability to manage workers go-vela/cli#430

Merged

ecrupper approved these changes Apr 10, 2023

View reviewed changes

ecrupper merged commit a7d3686 into main Apr 10, 2023

ecrupper deleted the log-paging branch April 10, 2023 18:41

colindean mentioned this pull request Aug 28, 2023

Increase Starlark execution limit, abstract limit resolver colindean/server#1

Closed

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

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

Uh oh!

Conversation

cognifloyd commented Mar 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Mar 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

wass3rw3rk left a comment

Choose a reason for hiding this comment

Uh oh!

jbrockopp left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jbrockopp left a comment

Choose a reason for hiding this comment

Uh oh!

jbrockopp left a comment

Choose a reason for hiding this comment

Uh oh!

jbrockopp commented Mar 28, 2023

Uh oh!

cognifloyd commented Mar 28, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

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

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

cognifloyd commented Mar 24, 2023 •

edited

Loading

codecov bot commented Mar 24, 2023 •

edited

Loading