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.

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

main: add --progres (similar to bib) #21

Merged
merged 5 commits into from
Jan 29, 2025
Merged

Conversation

mvo5
Copy link
Collaborator

@mvo5 mvo5 commented Dec 19, 2024

[draft as this lacks tests, not easy to do right now without duplicating code from bib, looking into this]

This commit add progress reporting similar to what bib is doing.
It imports the progress from bootc-image-builder for now. There
is an open PR for moving this to images but there are some
concerns about moving it there so for now we just use bib.
This is not too bad because eventually bib and ibcli will merge.

@supakeen
Copy link
Member

Very awesome and works for me; can we get rid of the WARN[0000] Failed to load consumer certs: no consumer key found?

@mvo5 mvo5 force-pushed the progress-silly-copied branch 3 times, most recently from 3bb8c3a to bfaef9f Compare January 27, 2025 09:55
@mvo5 mvo5 changed the title demo: add progres (similar to bib) main: add --progres (similar to bib) Jan 27, 2025
@supakeen
Copy link
Member

SRPM builds are failing due to the merge conflict in go.sum; otherwise I think this can be undrafted now that we use bib's code?

@mvo5 mvo5 force-pushed the progress-silly-copied branch from f24ee7a to 46f31a9 Compare January 27, 2025 16:42
@mvo5 mvo5 marked this pull request as ready for review January 27, 2025 16:42
@mvo5
Copy link
Collaborator Author

mvo5 commented Jan 27, 2025

Very awesome and works for me; can we get rid of the WARN[0000] Failed to load consumer certs: no consumer key found?

This should fix that particular nit: osbuild/bootc-image-builder#810

I marked this ready for review now, it contains a sad commit with some duplicated container building code (small but still sad) but this should make it ready and it unblocks the --with-buildlog in #106

@supakeen
Copy link
Member

supakeen commented Jan 27, 2025

Very awesome and works for me; can we get rid of the WARN[0000] Failed to load consumer certs: no consumer key found?

This should fix that particular nit: osbuild/bootc-image-builder#810

I marked this ready for review now, it contains a sad commit with some duplicated container building code (small but still sad) but this should make it ready and it unblocks the --with-buildlog in #106

I'm not too sad about it, knowing that it's hopefully temporary. Having the progress reporting is very nice.

The test is failing due to no longer getting the error on stderr bits in the output for TestBuildIntegration. This seems related to osbuild/bootc-image-builder#810 ?

@mvo5
Copy link
Collaborator Author

mvo5 commented Jan 28, 2025

Very awesome and works for me; can we get rid of the WARN[0000] Failed to load consumer certs: no consumer key found?

This should fix that particular nit: osbuild/bootc-image-builder#810
I marked this ready for review now, it contains a sad commit with some duplicated container building code (small but still sad) but this should make it ready and it unblocks the --with-buildlog in #106

I'm not too sad about it, knowing that it's hopefully temporary. Having the progress reporting is very nice.

The test is failing due to no longer getting the error on stderr bits in the output for TestBuildIntegration. This seems related to osbuild/bootc-image-builder#810 ?

Thanks, nice catch! I will need to update the test, the facts that we just forwarded stderr from osbuild unfiltered is a bit of a misfeature, I think for progress=term we can/should fix that, for progress=verbose I need to look, I think there should be compatibility code.

go.mod Show resolved Hide resolved
@mvo5
Copy link
Collaborator Author

mvo5 commented Jan 29, 2025

Very awesome and works for me; can we get rid of the WARN[0000] Failed to load consumer certs: no consumer key found?

This should fix that particular nit: osbuild/bootc-image-builder#810
I marked this ready for review now, it contains a sad commit with some duplicated container building code (small but still sad) but this should make it ready and it unblocks the --with-buildlog in #106

I'm not too sad about it, knowing that it's hopefully temporary. Having the progress reporting is very nice.
The test is failing due to no longer getting the error on stderr bits in the output for TestBuildIntegration. This seems related to osbuild/bootc-image-builder#810 ?

Thanks, nice catch! I will need to update the test, the facts that we just forwarded stderr from osbuild unfiltered is a bit of a misfeature, I think for progress=term we can/should fix that, for progress=verbose I need to look, I think there should be compatibility code.

The test is updated now, things are a little bit more complicated than I had hoped but its not too bad, the tests now nicely check for the error handling if osbuild fails which I think is a good thing (tm).

@mvo5 mvo5 force-pushed the progress-silly-copied branch 2 times, most recently from 7c8b9ca to 863cfb7 Compare January 29, 2025 12:13
mvo5 added 5 commits January 29, 2025 13:20
This commit add progress reporting similar to what `bib` is doing.
It imports the progress from `bootc-image-builder` for now. There
is an open PR for moving this to images but there are some
concerns about moving it there so for now we just use bib.
This is not too bad because eventually bib and ibcli will merge.
This is a very sad commit, it copies the code to make a fake
container with a faked osbuild.

The bright side is that the plan is to rewrite these helpers
in go and then we can just have a shared testutil module that
can be imported from both bib/ibcli.

And/or the two repos will merge and its just a (simple) test
helper. But I do not feel good (at all) about this.
This commit checks that the `--progress` argument generates the
expected output, i.e. with `term` we will not get any osbuild
output and the spinner. And with `verbose` we will not get a
spinner and the osbuild output.
This commit adds a new testutil.CaptureStdio helper so that we
can test external go modules that use os.Std{out,err} but now
allow mocking or overwriting.
This commit updates the tests for the error reporting. Since
we are now using the new "progress" module we can no longer
directly mock osStderr. So this now uses the new testutil
helper `CaptureStdio`.

Note also that the behavior of `verbose` and `term` is slightly
different - for backward compatibility in `verbose` mode the
stderr from osbuild is directly connected to the caller stderr.

For term mode this is not done to prevent spurious message from
leaking into the terminal progress and the full error is available
from the actual go error as captured output.
@mvo5 mvo5 force-pushed the progress-silly-copied branch from 863cfb7 to 639d44e Compare January 29, 2025 12:20
Copy link
Member

@supakeen supakeen left a comment

Choose a reason for hiding this comment

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

Very cool; some minor tidbits that could be changed but nothing that is a blocker. Seems like the Go version things have been resolved as well so we're ready to go!

@mvo5 mvo5 added this pull request to the merge queue Jan 29, 2025
Merged via the queue into osbuild:main with commit f46ca14 Jan 29, 2025
27 of 28 checks passed
@mvo5 mvo5 deleted the progress-silly-copied branch January 29, 2025 15:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants