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

feat: extend withProgress API to use task manager widget #2187

Merged
merged 3 commits into from
Apr 26, 2023

Conversation

jeffmaury
Copy link
Contributor

@jeffmaury jeffmaury commented Apr 20, 2023

Fixes #2019

What does this PR do?

Extend the withProgress API to report progress in the task API

Screenshot/screencast of this PR

progress

What issues does this PR fix or reference?

Fixes #2019

How to test this PR?

Re install Kind and Kind download should be reported in the task manager

Fixes containers#2019

Signed-off-by: Jeff MAURY <jmaury@redhat.com>
@jeffmaury jeffmaury requested a review from evidolob April 20, 2023 17:18
Signed-off-by: Jeff MAURY <jmaury@redhat.com>
@jeffmaury jeffmaury marked this pull request as ready for review April 20, 2023 20:37
@jeffmaury jeffmaury requested review from a team and benoitf as code owners April 20, 2023 20:37
@jeffmaury jeffmaury requested review from cdrage and lstocchi and removed request for a team April 20, 2023 20:37
Copy link
Contributor

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

@cdrage cdrage left a comment

Choose a reason for hiding this comment

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

LGTM! Don't see any issues with the code and was able to test it fine.

Feel free to merge whenever.

Copy link
Collaborator

@benoitf benoitf left a comment

Choose a reason for hiding this comment

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

I'm wondering if we should not remove ProgressLocation for APP_ICON but that's a different story.

It's not related to the withProgress API implementation using tasks but it seems the message that we display for kind is not accurate
We display in one side that we've downloaded kind but task is at 80% waiting that we install or not the binary in the PATH but the title is 'Downloading kind' so this task is over and it should be 100%
Or task should be 'Installing Kind' or two tasks I don't know but here it feels inconsistent.

7VRu3RXaZY.mp4

@jeffmaury
Copy link
Contributor Author

I'm wondering if we should not remove ProgressLocation for APP_ICON but that's a different story.

It's not related to the withProgress API implementation using tasks but it seems the message that we display for kind is not accurate We display in one side that we've downloaded kind but task is at 80% waiting that we install or not the binary in the PATH but the title is 'Downloading kind' so this task is over and it should be 100% Or task should be 'Installing Kind' or two tasks I don't know but here it feels inconsistent.
7VRu3RXaZY.mp4

I discussed with @evidolob and we choose to keep APP_ICON for a while. Maybe we should mark it deprecated and remove it after 0.15.0 ?

Will update the implementation so that a completed task is always 100%

@evidolob
Copy link
Contributor

I'm +1 to keep APP_ICON for now, mark it deprecated and delete on next release

Signed-off-by: Jeff MAURY <jmaury@redhat.com>
Copy link
Contributor

@evidolob evidolob left a comment

Choose a reason for hiding this comment

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

LGTM

@jeffmaury jeffmaury merged commit 38e9abe into containers:main Apr 26, 2023
7 checks passed
@jeffmaury jeffmaury deleted the GH-2019 branch April 26, 2023 15:21
@podman-desktop-bot podman-desktop-bot added this to the 0.15.0 milestone Apr 26, 2023
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.

Expose tasks to extension API
6 participants