Integrate a spinner to track time taken for long running operations #679 #680

priyanshikhetwani · 2024-10-04T04:48:45Z

**Summary: ** Integrate a spinner to track time taken for long running operations. #679

What Happened: Multiple logs with the same message is printed on screen which is redundant by nature, a spinner can be used to achieve the objective.

I0927 03:24:02.531498  835123 import.go:258] Importing image rhel-94-04242024-tier0. Please wait...
I0927 03:24:06.596504  835123 import.go:277] Image import is in-progress, current state: queued
I0927 03:26:06.680951  835123 import.go:277] Image import is in-progress, current state: running
I0927 03:28:06.623011  835123 import.go:277] Image import is in-progress, current state: running
I0927 03:30:06.843204  835123 import.go:277] Image import is in-progress, current state: running
I0927 03:32:06.727223  835123 import.go:277] Image import is in-progress, current state: running
I0927 03:34:06.769074  835123 import.go:277] Image import is in-progress, current state: running
I0927 03:36:06.920080  835123 import.go:277] Image import is in-progress, current state: running
I0927 03:38:06.861958  835123 import.go:277] Image import is in-progress, current state: running
I0927 03:40:06.951969  835123 import.go:277] Image import is in-progress, current state: running
I0927 03:42:06.826810  835123 import.go:277] Image import is in-progress, current state: running
I0927 03:44:06.873676  835123 import.go:277] Image import is in-progress, current state: running
I0927 03:46:06.901292  835123 import.go:271] Image imported successfully, took 22m0.670579377s
Rather a spinner can be set-up lo track the current status and the time elapsed to improve user experience.

What you expected to happen:
A single line of log to track the status.

**Test: **
Example Output During Polling:

ppc64le-cloud-bot · 2024-10-04T04:48:54Z

Welcome @priyanshikhetwani! It looks like this is your first PR to ppc64le-cloud/pvsadm 🎉

ppc64le-cloud-bot · 2024-10-04T04:48:54Z

Hi @priyanshikhetwani. Thanks for your PR.

I'm waiting for a ppc64le-cloud member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

pkg/utils/utils.go

go.mod

pkg/utils/utils.go

cmd/image/import/import.go

pkg/utils/utils.go

kishen-v · 2024-10-11T06:22:49Z

/ok-to-test

kishen-v · 2024-10-11T06:48:19Z

Fixes: #679

kishen-v · 2024-10-29T04:09:31Z

@priyanshikhetwani, I see there are merge conflicts, can you PTAL?
Thanks!

kishen-v · 2024-10-29T09:01:12Z

/lgtm
Thanks.

mkumatag · 2024-11-04T08:28:07Z

pkg/utils/utils.go

@@ -56,17 +58,38 @@ func RetrieveValFromMap[K comparable, V any](m map[K]V, key K) V {
 // PollUntil validates if a certain condition is met at defined poll intervals.
 // If a timeout is reached, an associated error is returned to the caller.
 // condition contains the use-case specific code that returns true when a certain condition is achieved.
-func PollUntil(pollInterval, timeOut <-chan time.Time, condition func() (bool, error)) error {
+func PollUntil(pollInterval, timeOut <-chan time.Time, condition func() (string, bool, error)) error {


can we have different function which uses spinner and leave this default one as is? may be like SpinnerPollUntil

Yes, makes sense since we are changing the function's signature

mkumatag · 2024-11-04T08:36:59Z

pkg/utils/utils.go

+			timeoutMessage := " timed out while waiting for job to complete"
+			s.Suffix = timeoutMessage


Suggested change

timeoutMessage := " timed out while waiting for job to complete"

s.Suffix = timeoutMessage

s.Suffix = " timed out while waiting for job to complete"

mkumatag · 2024-11-04T08:41:27Z

cmd/image/import/import.go

@@ -262,20 +262,20 @@ pvsadm image import -n upstream-core-lon04 -b <BUCKETNAME> --object rhel-83-1003
 			return err
 		}
 		start := time.Now()
-		err = utils.PollUntil(time.Tick(2*time.Minute), time.After(opt.WatchTimeout), func() (bool, error) {
+		var message string


Will making this variable available to the block below work?

This variable is accessed and modified outside the block as well

I don't think so, @kishen-v can you please check

Sure Manju, I'll take a look at it.

Hi @mkumatag, I see that it's reused here:
https://github.com/ppc64le-cloud/pvsadm/pull/680/files#diff-ae8fe9bef8e5a7ec3362a0d4dca4f3e432f4b1fe64e7c1dde9b23d98dbc8f937R308-R309

Or, we could switch to short-hand declaration in both cases. Just a thought..

why are we reusing it? are we sharing anything between those 2 blocks? if not - lets define locally.

Added short-hand declaration, since we aren't sharing anything between the 2 blocks

mkumatag · 2024-11-07T04:38:27Z

go.mod

 	gopkg.in/tomb.v1 v1.0.0-20141024135613-dd632973f1e7 // indirect
 	gopkg.in/yaml.v3 v3.0.1 // indirect
-)
+)


add the additional newline

cc @kishen-v

mkumatag · 2024-11-07T04:39:46Z

cmd/image/import/import.go

@@ -262,20 +262,20 @@ pvsadm image import -n upstream-core-lon04 -b <BUCKETNAME> --object rhel-83-1003
 			return err
 		}
 		start := time.Now()
-		err = utils.PollUntil(time.Tick(2*time.Minute), time.After(opt.WatchTimeout), func() (bool, error) {
+		var message string


I don't think so, @kishen-v can you please check

mkumatag

/lgtm

ppc64le-cloud-bot · 2024-11-08T06:33:47Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mkumatag, priyanshikhetwani

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~~ [mkumatag]

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

ppc64le-cloud-bot requested review from bkhadars and mkumatag October 4, 2024 04:48

ppc64le-cloud-bot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Oct 4, 2024

kishen-v reviewed Oct 4, 2024

View reviewed changes

pkg/utils/utils.go Outdated Show resolved Hide resolved

priyanshikhetwani changed the title ~~Integrate a spinner to track time taken for long running operations #679~~ WIP: Integrate a spinner to track time taken for long running operations #679 Oct 4, 2024

ppc64le-cloud-bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 4, 2024

kishen-v reviewed Oct 7, 2024

View reviewed changes

pkg/utils/utils.go Outdated Show resolved Hide resolved

kishen-v reviewed Oct 7, 2024

View reviewed changes

pkg/utils/utils.go Outdated Show resolved Hide resolved

kishen-v reviewed Oct 7, 2024

View reviewed changes

pkg/utils/utils.go Outdated Show resolved Hide resolved

mkumatag reviewed Oct 8, 2024

View reviewed changes

go.mod Outdated Show resolved Hide resolved

mkumatag reviewed Oct 9, 2024

View reviewed changes

go.mod Outdated Show resolved Hide resolved

kishen-v reviewed Oct 9, 2024

View reviewed changes

pkg/utils/utils.go Outdated Show resolved Hide resolved

kishen-v reviewed Oct 9, 2024

View reviewed changes

cmd/image/import/import.go Outdated Show resolved Hide resolved

kishen-v reviewed Oct 9, 2024

View reviewed changes

cmd/image/import/import.go Outdated Show resolved Hide resolved

kishen-v reviewed Oct 9, 2024

View reviewed changes

cmd/image/import/import.go Outdated Show resolved Hide resolved

kishen-v reviewed Oct 9, 2024

View reviewed changes

cmd/image/import/import.go Outdated Show resolved Hide resolved

kishen-v reviewed Oct 9, 2024

View reviewed changes

pkg/utils/utils.go Show resolved Hide resolved

priyanshikhetwani force-pushed the feature/integrate-spinner branch 2 times, most recently from 81eba62 to 4e1a69c Compare October 10, 2024 13:36

priyanshikhetwani changed the title ~~WIP: Integrate a spinner to track time taken for long running operations #679~~ Integrate a spinner to track time taken for long running operations #679 Oct 10, 2024

ppc64le-cloud-bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 10, 2024

priyanshikhetwani force-pushed the feature/integrate-spinner branch from 4e1a69c to 0dfbd9f Compare October 10, 2024 13:56

kishen-v reviewed Oct 10, 2024

View reviewed changes

pkg/utils/utils.go Outdated Show resolved Hide resolved

kishen-v reviewed Oct 10, 2024

View reviewed changes

pkg/utils/utils.go Outdated Show resolved Hide resolved

priyanshikhetwani force-pushed the feature/integrate-spinner branch from 0dfbd9f to a0cdf44 Compare October 11, 2024 06:13

ppc64le-cloud-bot added the ok-to-test Indicates a non-member PR verified by an org member that is safe to test. label Oct 11, 2024

ppc64le-cloud-bot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Oct 11, 2024

priyanshikhetwani force-pushed the feature/integrate-spinner branch 3 times, most recently from 1e9493e to 6573309 Compare October 29, 2024 05:57

ppc64le-cloud-bot assigned kishen-v Oct 29, 2024

ppc64le-cloud-bot added the lgtm Indicates that a PR is ready to be merged. label Oct 29, 2024

mkumatag requested changes Nov 4, 2024

View reviewed changes

priyanshikhetwani force-pushed the feature/integrate-spinner branch from 6573309 to 6030be4 Compare November 5, 2024 20:42

ppc64le-cloud-bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed lgtm Indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Nov 5, 2024

priyanshikhetwani force-pushed the feature/integrate-spinner branch from 6030be4 to b980053 Compare November 5, 2024 21:09

ppc64le-cloud-bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Nov 5, 2024

mkumatag requested changes Nov 7, 2024

View reviewed changes

added spinner for long running tasks

282360f

priyanshikhetwani force-pushed the feature/integrate-spinner branch from 11f51eb to 282360f Compare November 8, 2024 06:18

mkumatag approved these changes Nov 8, 2024

View reviewed changes

ppc64le-cloud-bot assigned mkumatag Nov 8, 2024

ppc64le-cloud-bot added the lgtm Indicates that a PR is ready to be merged. label Nov 8, 2024

ppc64le-cloud-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 8, 2024

ppc64le-cloud-bot merged commit 587a894 into ppc64le-cloud:main Nov 8, 2024
2 checks passed

This was referenced Nov 8, 2024

Validate conditions immediately upon invocation. #678

Closed

Enhancement: Integrate a spinner to track time taken for long running operations. #679

Closed

Enhancement: Check if a condition is met before the polling interval has elapsed. #676

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Integrate a spinner to track time taken for long running operations #679 #680

Integrate a spinner to track time taken for long running operations #679 #680

priyanshikhetwani commented Oct 4, 2024 •

edited

Loading

ppc64le-cloud-bot commented Oct 4, 2024

ppc64le-cloud-bot commented Oct 4, 2024

kishen-v commented Oct 11, 2024

kishen-v commented Oct 11, 2024

kishen-v commented Oct 29, 2024

kishen-v commented Oct 29, 2024

mkumatag Nov 4, 2024

priyanshikhetwani Nov 5, 2024

mkumatag Nov 4, 2024

priyanshikhetwani Nov 6, 2024

mkumatag Nov 4, 2024

priyanshikhetwani Nov 6, 2024

mkumatag Nov 7, 2024

kishen-v Nov 8, 2024

kishen-v Nov 8, 2024

mkumatag Nov 8, 2024

priyanshikhetwani Nov 8, 2024

mkumatag Nov 7, 2024

priyanshikhetwani Nov 8, 2024

mkumatag Nov 7, 2024

mkumatag left a comment

ppc64le-cloud-bot commented Nov 8, 2024

		timeoutMessage := " timed out while waiting for job to complete"
		s.Suffix = timeoutMessage

	timeoutMessage := " timed out while waiting for job to complete"
	s.Suffix = timeoutMessage
	s.Suffix = " timed out while waiting for job to complete"

Integrate a spinner to track time taken for long running operations #679 #680

Integrate a spinner to track time taken for long running operations #679 #680

Conversation

priyanshikhetwani commented Oct 4, 2024 • edited Loading

ppc64le-cloud-bot commented Oct 4, 2024

ppc64le-cloud-bot commented Oct 4, 2024

kishen-v commented Oct 11, 2024

kishen-v commented Oct 11, 2024

kishen-v commented Oct 29, 2024

kishen-v commented Oct 29, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mkumatag left a comment

Choose a reason for hiding this comment

ppc64le-cloud-bot commented Nov 8, 2024

priyanshikhetwani commented Oct 4, 2024 •

edited

Loading