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

deploy: Add a spinner #848

Merged
merged 3 commits into from
Nov 5, 2024
Merged

Conversation

cgwalters
Copy link
Collaborator

deploy: Use bar.println

In some cases I wasn't seeing this output, and I think it's
because we hadn't drop()'d the bar. Since it's in scope
it's cleaner to use its println method.

Signed-off-by: Colin Walters walters@verbum.org


deploy: Add a spinner

First, this operation was absolutely not asynchronous
and we need to spawn it in a helper thread on general
principle. Doing that is unfortunately VERY hacky
because we need to clone a lot of things and especially
there's an ergonomics hit here since Deployment is
incorrectly !Send.

But the main motivation is this operation can be slow sometimes,
so let's show progress output so the admin knows we're not blocked.

Closes: #842
Signed-off-by: Colin Walters walters@verbum.org


utils: Print and log time for async_task_with_spinner

Things we do via this mechanism can take some time; let's
log the elapsed time for greater visibility both to the
human output and log to the journal.

Signed-off-by: Colin Walters walters@verbum.org


In some cases I wasn't seeing this output, and I think it's
because we hadn't `drop()`'d the bar. Since it's in scope
it's cleaner to use its `println` method.

Signed-off-by: Colin Walters <walters@verbum.org>
First, this operation was absolutely not asynchronous
and we need to spawn it in a helper thread on general
principle. Doing that is unfortunately VERY hacky
because we need to clone a lot of things and especially
there's an ergonomics hit here since `Deployment` is
incorrectly `!Send`.

But the main motivation is this operation can be slow sometimes,
so let's show progress output so the admin knows we're not blocked.

Closes: containers#842
Signed-off-by: Colin Walters <walters@verbum.org>
Copy link
Member

@vrothberg vrothberg left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@omertuc omertuc left a comment

Choose a reason for hiding this comment

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

looks good except for one question

lib/src/deploy.rs Show resolved Hide resolved
@omertuc
Copy link
Contributor

omertuc commented Nov 5, 2024

/lgtm

@omertuc
Copy link
Contributor

omertuc commented Nov 5, 2024

Oh and FYI, IDK if you care but you added tracing::debug!("Deploying commit {ostree_commit}"); under a seemingly irrelevant commit (adc37ab)

Things we do via this mechanism can take some time; let's
log the elapsed time for greater visibility both to the
human output *and* log to the journal.

Signed-off-by: Colin Walters <walters@verbum.org>
@cgwalters
Copy link
Collaborator Author

Oh and FYI, IDK if you care but you added tracing::debug!("Deploying commit {ostree_commit}"); under a seemingly irrelevant commit (adc37ab)

I dropped that

@cgwalters
Copy link
Collaborator Author

/lgtm

This repository doesn't use Prow, so to merge it needs a Github approval state

@cgwalters cgwalters merged commit 07593a0 into containers:main Nov 5, 2024
34 checks passed
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.

Add progress/feedback for deploy
3 participants