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

pkg/test/e2eutil: wait for a deployment to have >= n replicas #2248

Merged

Conversation

dustinspecker
Copy link
Contributor

Before waitForDeployment was waiting until the exact number of replicas
were available. Now, wait for at least that number of replicas to be
available.

scenario:
I'm using an operator with 3 replicas, but only 1 is needed for tests to
work. If all 3 replicas were available before waitForDeployment started
then it would wait until it timed out because 3 does not equal 1. Now,
since 3 replicas is greater than the 1 needed for tests waitForDeployment
successfully waits.

@openshift-ci-robot openshift-ci-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Nov 20, 2019
Copy link
Contributor

@camilamacedo86 camilamacedo86 left a comment

Choose a reason for hiding this comment

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

It makes sense to me. Good catch. 🥇
It is just missing an entry in the CHANGELOG. Could you please add?

@dustinspecker
Copy link
Contributor Author

It makes sense to me. Good catch.
It is just missing an entry in the CHANGELOG. Could you please add?

Sorry about that - I've added it now.

CHANGELOG.md Outdated
@@ -11,6 +11,7 @@
### Removed

### Bug Fixes
- WaitForDeployment and WaitForOperatorDeployment from pkg/test/e2eutil successfully complete waiting when the available replica count is greater than the minimum replica count required. ([#2248](https://github.com/operator-framework/operator-sdk/pull/2248))
Copy link
Contributor

@camilamacedo86 camilamacedo86 Nov 21, 2019

Choose a reason for hiding this comment

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

Suggested change
- WaitForDeployment and WaitForOperatorDeployment from pkg/test/e2eutil successfully complete waiting when the available replica count is greater than the minimum replica count required. ([#2248](https://github.com/operator-framework/operator-sdk/pull/2248))
- Fix `WaitForDeployment` and `WaitForOperatorDeployment` from`pkg/test/e2eutil` successfully complete waiting when the available replica count is equals or greater than the minimum replica count required. ([#2248](https://github.com/operator-framework/operator-sdk/pull/2248))

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure this is a bug fix. Seems more like a change/improvement to me.

Also to build on @camilamacedo86's suggestion, WDYT of this for the CHANGELOG line:

Suggested change
- WaitForDeployment and WaitForOperatorDeployment from pkg/test/e2eutil successfully complete waiting when the available replica count is greater than the minimum replica count required. ([#2248](https://github.com/operator-framework/operator-sdk/pull/2248))
- Updated `pkg/test/e2eutil.WaitForDeployment()` and `pkg/test/e2eutil.WaitForOperatorDeployment()` to successfully complete waiting when the available replica count is _at least_ (rather then exactly) the minimum replica count required. ([#2248](https://github.com/operator-framework/operator-sdk/pull/2248))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That sounds good to me. I've updated the changelog. Thank you both for the help!

camilamacedo86
camilamacedo86 previously approved these changes Nov 21, 2019
Copy link
Contributor

@camilamacedo86 camilamacedo86 left a comment

Choose a reason for hiding this comment

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

They may have a better suggestion for the CHANGELOG. So, please feel free to wait for others input as well.

@camilamacedo86 camilamacedo86 dismissed their stale review November 21, 2019 03:14

Shows that after this change the asserts are not working well for ansible. See skipping: [localhost] => {"changed": false, "skip_reason": "Conditional result was False"}

Copy link
Contributor

@camilamacedo86 camilamacedo86 left a comment

Choose a reason for hiding this comment

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

/lgtm /approved
It is passing in the CI now
and it is a very small change which shows 100% ok for me.

@camilamacedo86 camilamacedo86 added approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. labels Nov 21, 2019
Before waitForDeployment was waiting until the exact number of replicas
were available. Now, wait for at least that number of replicas to be
available.

scenario:
I'm using an operator with 3 replicas, but only 1 is needed for tests to
work. If all 3 replicas were available before waitForDeployment started
then it would wait until it timed out because 3 does not equal 1. Now,
since 3 replicas is greater than the 1 needed for tests waitForDeployment
successfully waits.
@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Nov 22, 2019
Copy link
Contributor

@camilamacedo86 camilamacedo86 left a comment

Choose a reason for hiding this comment

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

/lgtm /approved

@joelanford are you ok with merge this one now? WDYT?

@joelanford
Copy link
Member

Looks good to me.

@hasbro17 @estroz We've had this code around for awhile without changing it, so just double checking. Can you think of any reason we shouldn't merge this change?

Copy link
Member

@estroz estroz left a comment

Choose a reason for hiding this comment

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

@joelanford no reason not to merge this.

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Nov 25, 2019
@estroz estroz changed the title pkg/test/e2eutil: wait for a deployment to have at least # replicas pkg/test/e2eutil: wait for a deployment to have >= n replicas Nov 25, 2019
@estroz estroz merged commit 94a0669 into operator-framework:master Nov 25, 2019
@dustinspecker dustinspecker deleted the fix-wait-for-deployment branch November 25, 2019 18:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants