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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[supervisor] Better reflect incremental prebuilds in prebuild logs #4293

Merged
merged 1 commit into from
Jun 5, 2021
Merged

[supervisor] Better reflect incremental prebuilds in prebuild logs #4293

merged 1 commit into from
Jun 5, 2021

Conversation

jankeromnes
Copy link
Contributor

@jankeromnes jankeromnes commented May 25, 2021 •
edited
Loading

If a prebuild is based on another prebuild, we currently truncate & overwrite the prebuild logs, and "forget" about how long the parent prebuild took.

Instead, we should:

  • Keep the parent prebuild logs (to be more transparent about incremental prebuilds and allow debugging builds)
  • Log the total saved time (parent prebuild + incremental prebuild)

That's what this PR does.

How to test

  1. Click here to trigger a full (parent) prebuild -- the printed terminal logs should look like a normal prebuild:
    • "馃" message
    • but no "鈾伙笍" message
  2. Then click here to trigger an incremental prebuild -- now the printed terminal logs should include:
    • parent prebuild logs
    • "鈾伙笍" message
    • incremental prebuild logs
    • combined "馃" message
  3. Click here to trigger a long (> 1 min) full (parent) prebuild -- after the prebuild is done, the terminal should have:
    • "馃"
    • "馃帀" (1 min)
    • but no "鈾伙笍"
  4. Then click here to trigger a long (> 1 min) incremental prebuild -- when it's done, terminal should have:
    • parent logs
    • "鈾伙笍"
    • new logs
    • "馃"
    • " 馃帀" (still 1 min, not 2)

@jankeromnes jankeromnes force-pushed the jx/incremental-prebuild-logs branch from 959ffc1 to 7d82037 Compare May 25, 2021 17:03
@jankeromnes jankeromnes marked this pull request as ready for review May 25, 2021 17:03
@jankeromnes jankeromnes mentioned this pull request May 25, 2021
Merged
csweichel
csweichel reviewed May 26, 2021
View reviewed changes
components/supervisor/pkg/supervisor/tasks.go Outdated Show resolved Hide resolved
components/supervisor/pkg/supervisor/tasks.go Outdated Show resolved Hide resolved
components/supervisor/pkg/supervisor/tasks.go Outdated Show resolved Hide resolved
@jankeromnes jankeromnes force-pushed the jx/incremental-prebuild-logs branch 3 times, most recently from 4e6a4dd to d35e81e Compare May 26, 2021 13:24
jankeromnes
jankeromnes commented May 27, 2021
View reviewed changes
Copy link
Contributor Author

@jankeromnes jankeromnes left a comment

Choose a reason for hiding this comment

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

Transcribed feedback from 1:1 call.

components/supervisor/pkg/supervisor/tasks.go Outdated Show resolved Hide resolved
components/supervisor/pkg/supervisor/tasks.go Outdated Show resolved Hide resolved
components/supervisor/pkg/supervisor/tasks.go Outdated Show resolved Hide resolved
components/supervisor/pkg/supervisor/tasks.go Outdated Show resolved Hide resolved
components/supervisor/pkg/supervisor/tasks.go Outdated Show resolved Hide resolved
@jankeromnes jankeromnes force-pushed the jx/incremental-prebuild-logs branch from d35e81e to 36aa666 Compare May 28, 2021 13:02
@csweichel csweichel marked this pull request as draft May 31, 2021 06:59
@jankeromnes jankeromnes force-pushed the jx/incremental-prebuild-logs branch from 36aa666 to 5fb06bb Compare May 31, 2021 07:45
@jankeromnes
Copy link
Contributor Author

jankeromnes commented May 31, 2021 •
edited by werft-gitpod-dev-com bot
Loading

馃幇

/werft run

馃憤 started the job as gitpod-build-jx-incremental-prebuild-logs.8

@jankeromnes jankeromnes marked this pull request as ready for review May 31, 2021 09:37
@jankeromnes
Copy link
Contributor Author

Can't be tested (core-dev deployments are currently broken) but ready for review.

@jankeromnes
Copy link
Contributor Author

jankeromnes commented May 31, 2021 •
edited by werft-gitpod-dev-com bot
Loading

馃幇

/werft run

馃憤 started the job as gitpod-build-jx-incremental-prebuild-logs.9

@jankeromnes
Copy link
Contributor Author

jankeromnes commented Jun 1, 2021 •
edited by werft-gitpod-dev-com bot
Loading

馃幇

/werft run

馃憤 started the job as gitpod-build-jx-incremental-prebuild-logs.10

@jankeromnes
Copy link
Contributor Author

jankeromnes commented Jun 1, 2021 •
edited by werft-gitpod-dev-com bot
Loading

Works as expected! 馃帀

@csweichel please take another look 馃憖

Screenshot 2021-06-01 at 15 58 11

csweichel
csweichel reviewed Jun 3, 2021
View reviewed changes
Copy link
Contributor

@csweichel csweichel left a comment

Choose a reason for hiding this comment

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

I know this feels a bit like coming out of the blue, but a unit test for importParentLogAndGetDuration would be nice. If you moved the defer os.Remove() out of importParentLogAndGetDuration you could re-write that function to use the new io/fs package (testing/fstest in the test correspondingly).

Considering the increased emphasis on testing is a recent development, I'd be ok to approve this PR as is - but tests sure would be nice :)

components/supervisor/pkg/supervisor/tasks.go
)
if _, err := os.Stat(fileName); err == nil {
// If the file already exists (from a parent prebuild), temporarily move it so that it doesn't get truncated.
_ = os.Rename(fileName, oldFileName)
Copy link
Contributor

Choose a reason for hiding this comment

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

What happens if the rename fails? Considering the error is ignored here, a comment as to why ignoring it is fine would be handy

@jankeromnes jankeromnes force-pushed the jx/incremental-prebuild-logs branch 2 times, most recently from a085c69 to e6354de Compare June 5, 2021 08:05
@jankeromnes jankeromnes force-pushed the jx/incremental-prebuild-logs branch from e6354de to c0964d7 Compare June 5, 2021 08:07
@jankeromnes jankeromnes merged commit b6ce008 into main Jun 5, 2021
@jankeromnes jankeromnes deleted the jx/incremental-prebuild-logs branch June 5, 2021 08:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@csweichel csweichel csweichel approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@jankeromnes @csweichel