-
Notifications
You must be signed in to change notification settings - Fork 244
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
Do not display "Waiting for component to start" twice #4919
Do not display "Waiting for component to start" twice #4919
Conversation
@@ -56,6 +57,22 @@ func (a *Adapter) getPod(refresh bool) (*corev1.Pod, error) { | |||
if refresh || a.pod == nil { | |||
podSelector := fmt.Sprintf("component=%s", a.ComponentName) | |||
|
|||
// First check if the pod is already in desired state | |||
c := a.Client.GetKubeClient() | |||
podList, err := c.KubeClient.CoreV1().Pods(c.Namespace).List(context.TODO(), metav1.ListOptions{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can avoid listing the pods and use the condition appsv1.DeploymentAvailable
in the WaitAndGetPodWithEvents
function to start and stop the spinner
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In fact, I cannot see any use case where this function getPod is really waiting for the pod to be started. @mik-dass do you have any example when this function is waiting?
The only function I can see waiting is WaitForDeploymentRollout
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I changed to start the spinner only when a phase different from the expected one is received
7ab7f3c
to
b131232
Compare
b131232
to
a4b06f4
Compare
Kudos, SonarCloud Quality Gate passed! 0 Bugs No Coverage information |
/retest |
defer spinner.End(false) | ||
var spinner *log.Status | ||
defer func() { | ||
if spinner != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In what situation will this check be not satisfied? We're doing var spinner *log.Status
right above this so, I'm confused as to when spinner
will be nil
and this check would be not satisfied.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In var spinner *log.Status
, spinner is a non initialized pointer, so getting its zero value, nil.
The pointer gets affected a value only when receiving a phase different from the expected one.
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: kadel 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:
Approvers can indicate their approval by writing |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
…er#4919) * Do not display "Waiting for component to start" twice * Changelog
What type of PR is this?
/kind bug
What does this PR do / why we need it:
WaitAndGetPodWithEvents
now waits for an event with a Phase different from the expected one before to start the Spinner. This way, the spinner is only displayed when the pod is not in the expected phase.Which issue(s) this PR fixes:
Fixes #4362
PR acceptance criteria:
Unit test
Integration test
Documentation
Update changelog
I have read the test guidelines
How to test changes / Special notes to the reviewer: