Fixes #3546 #3550 Add option to allow progressbars to be forced during download and uncompression of the bundle (setup phase)

[gbraad]

Fixes #3546 #3550

As part of crc setup we show the progress of the download and
uncompression. However, the download progress is always shown, while
the uncompression is suppressed when this is not a standard terninal.
For consistency we remove the suppression as this adds unnecessary
complexity.

[cfergeau]

Imo, this makes things worse for :

% crc setup &>foo.log

% cat foo.log 
level=info msg="Using bundle path /Users/teuf/.crc/cache/crc_vfkit_4.12.5_arm64.crcbundle"
level=info msg="Checking if running as non-root"
level=info msg="Checking if crc-admin-helper executable is cached"
level=info msg="Checking if running on a supported CPU architecture"
level=info msg="Checking minimum RAM requirements"
level=info msg="Checking if crc executable symlink exists"
level=info msg="Checking if running emulated on a M1 CPU"
level=info msg="Checking if vfkit is installed"
level=info msg="Checking if CRC bundle is extracted in '$HOME/.crc'"
level=info msg="Checking if /Users/teuf/.crc/cache/crc_vfkit_4.12.5_arm64.crcbundle exists"
level=info msg="Getting bundle for the CRC executable"
level=info msg="Downloading bundle: /Users/teuf/.crc/cache/crc_vfkit_4.12.5_arm64.crcbundle..."
0 B / 3.15 GiB [___________________________________________________] 0.00% ? p/s0 B / 3.15 GiB [___________________________________________________] 0.00% ? p/s339.45 MiB / 3.15 GiB [---->______________________________________] 10.51% ? p/s339.45 MiB / 3.15 GiB [--->______________________________] 10.51% 565.86 MiB p/s339.45 MiB / 3.15 GiB [--->______________________________] 10.51% 565.86 MiB p/s347.05 MiB / 3.15 GiB [--->______________________________] 10.75% 565.86 MiB p/s347.05 MiB / 3.15 GiB [--->______________________________] 10.75% 530.17 MiB p/s354.38 MiB / 3.15 GiB [--->______________________________] 10.97% 530.17 MiB p/s354.38 MiB / 3.15 GiB [--->______________________________] 10.97% 530.17 MiB p/s362.77 MiB / 3.15 GiB [--->______________________________] 11.23% 497.66 MiB p/s362.77 MiB / 3.15 GiB [--->______________________________] 11.23% 497.66 MiB p/s

[praveenkumar]

Imo, this makes things worse for :

@cfergeau which is the current behavior right even without this PR? Also isn't it expected even with our current terminal check since crc is going to detect it as terminal and show the progress bar but here we are just sending it to a log file?

[cfergeau]

@cfergeau which is the current behavior right even without this PR?

Even if it's the current behaviour, it does not make it correct, especially when other parts of the code don't have the same behaviour.

IsTerminal will detect when there's a redirection to a file, with the quick hack below, the log file looks like this during the download:

level=info msg="Checking if CRC bundle is extracted in '$HOME/.crc'"
level=info msg="Checking if /home/teuf/.crc/cache/crc_libvirt_4.12.5_amd64.crcbundle exists"
level=info msg="Getting bundle for the CRC executable"
level=info msg="Downloading bundle: /home/teuf/.crc/cache/crc_libvirt_4.12.5_amd64.crcbundle..."

diff --git a/pkg/download/download.go b/pkg/download/download.go
index 532b5f4da..1a0ecdfef 100644
--- a/pkg/download/download.go
+++ b/pkg/download/download.go
@@ -15,6 +15,7 @@ import (
        "github.com/cavaliergopher/grab/v3"
        "github.com/cheggaaa/pb/v3"
        "github.com/pkg/errors"
+       terminal "golang.org/x/term"
 )
 
 func doRequest(client *grab.Client, req *grab.Request) (string, error) {
@@ -28,15 +29,20 @@ func doRequest(client *grab.Client, req *grab.Request) (string, error) {
 
        t := time.NewTicker(500 * time.Millisecond)
        defer t.Stop()
-       bar := pb.Start64(resp.Size())
-       bar.Set(pb.Bytes, true)
-       defer bar.Finish()
+       var bar *pb.ProgressBar
+       if terminal.IsTerminal(int(os.Stdout.Fd())) {
+               bar = pb.Start64(resp.Size())
+               bar.Set(pb.Bytes, true)
+               defer bar.Finish()
+       }
 
 loop:
        for {
                select {
                case <-t.C:
-                       bar.SetCurrent(resp.BytesComplete())
+                       if terminal.IsTerminal(int(os.Stdout.Fd())) {
+                               bar.SetCurrent(resp.BytesComplete())
+                       }
                case <-resp.Done:
                        break loop
                }

[praveenkumar]

IsTerminal will detect when there's a redirection to a file, with the quick hack below, the log file looks like this during the download

yes but what we want to do is to show the progress bar in every case for PD extension usecase or you are saying we should use this and add a bool flag --show-progressbar at top level and then use it with setup and start ?

[cfergeau]

yes but what we want to do is to show the progress bar in every case for PD extension usecase or you are saying we should use this and add a bool flag --show-progressbar at top level and then use it with setup and start ?

I'd prefer this yes, by default, we have --show-progressbar=auto, and whether to show a progress bar or not will depend on IsTerminal(stdout). The podman-desktop extension can use --show-progressbar=yes to get it shown even if it's not a terminal.

[gbraad]

I think the argument you give is a little moot... This is not behaviour we normally suggest to use; file redirection for the setup flow.

Imo, this makes things worse for :

Besides, you are judging about DOWNLOADING that has been untouched in this PR, so this is existing behaviour.

Even if it's the current behaviour, it does not make it correct

However, this makes every comment justified and PRs linger for unnecessary long times due to enlarged scope.

---

I suggest a follow-up issue to 'fix' this for downloading, as this is not part of this issue/PR : #3550 The suggested fixes will not be part of the current PR.

[praveenkumar]

Current PR also need to be fixed to run unit tests which is failing because of https://github.com/crc-org/crc/pull/3547/files#diff-2f2c46f41491d3331c52245381a6bf0a66bf5b8d708bb3018157764e7b744e50R20 one.

[gbraad]

I am looking at adding the requested complexity to handle 'intelligent' showing of the progress.

[cfergeau]

This is not behaviour we normally suggest to use; file redirection for the setup flow.

We ask for logs for most issues filed, file redirection is one way of capturing these. If file redirection is never something we want the user to do, let's document this, and remove all usage of IsTerminal/RunningInTerminal from crc.

Besides, you are judging about DOWNLOADING that has been untouched in this PR, so this is existing behaviour.

I know, but downloading and uncompressing currently behave differently, we disagree on which one need to be changed and fixed, hence the mention of the one I would change.

[gbraad]

currently behave differently,

Hence, addressed to remove complexity

hence the mention of the one I would change.

Unfortunately, the suggestion is not in line with the schedule we have. We should be more pragmatic and consider breaking down issues and work on smaller increment by improvements issues instead of waxing and polishing PRs into perfection.

We ask for logs for most issues filed

I look forward to the modification of the issue template to also include:

$ crc setup --log-level debug

[cfergeau]

Thanks for making this configurable!

[gbraad]

Not sure why the test fails with:

pkg/extract/extract.go:20:2: "golang.org/x/term" imported but not used as terminal (typecheck)
	terminal "golang.org/x/term"

as this file does not contain a reference to x/term

[openshift-ci]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cfergeau

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [cfergeau]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[gbraad]

has been verified by @evidolob to provide the progressbars with crc setup --show-progressbars