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.

Sign up for GitHub

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

Jump to bottom

apps: extend extended tests to better check for deployer invariants and enable back the old check #16998

Merged
merged 5 commits into from
Nov 30, 2017
Merged

apps: extend extended tests to better check for deployer invariants and enable back the old check #16998

merged 5 commits into from
Nov 30, 2017

Conversation

tnozicka
Copy link
Contributor

@tnozicka tnozicka commented Oct 23, 2017 •
edited

enabling back #16956

@openshift-ci-robot openshift-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Oct 23, 2017
@tnozicka tnozicka assigned tnozicka and unassigned stevekuznetsov Oct 23, 2017
@tnozicka tnozicka force-pushed the fix-multiple-deployer-pods-due-to-cancellation branch from 39957d6 to 7324453 Compare October 24, 2017 16:34
@tnozicka tnozicka changed the title [DO NOT MERGE][WIP] test - disregard apps: extend extended test to better check for deployer invariants and enable back the old check Oct 24, 2017
@tnozicka tnozicka changed the title apps: extend extended test to better check for deployer invariants and enable back the old check apps: extend extended tests to better check for deployer invariants and enable back the old check Oct 24, 2017
@tnozicka
Copy link
Contributor Author

this is currently blocked on #17011

@tnozicka tnozicka added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. and removed do-not-merge labels Oct 24, 2017
@tnozicka tnozicka changed the title apps: extend extended tests to better check for deployer invariants and enable back the old check [WIP] apps: extend extended tests to better check for deployer invariants and enable back the old check Oct 24, 2017
@openshift-ci-robot openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 24, 2017
@tnozicka tnozicka force-pushed the fix-multiple-deployer-pods-due-to-cancellation branch from 7324453 to e8cad12 Compare October 24, 2017 17:31
@tnozicka tnozicka changed the title [WIP] apps: extend extended tests to better check for deployer invariants and enable back the old check apps: extend extended tests to better check for deployer invariants and enable back the old check Oct 24, 2017
@openshift-ci-robot openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 24, 2017
@tnozicka tnozicka requested a review from mfojtik October 24, 2017 17:33
@tnozicka
Copy link
Contributor Author

@mfojtik this is now ready for your review

mfojtik
mfojtik reviewed Oct 24, 2017
View reviewed changes
pkg/apps/controller/deploymentconfig/deploymentconfig_controller.go
@@ -121,20 +121,21 @@ func (c *DeploymentConfigController) Handle(config *deployapi.DeploymentConfig)
return c.updateStatus(config, existingDeployments)
}

latestIsDeployed, latestDeployment := deployutil.LatestDeploymentInfo(config, existingDeployments)
latestExists, latestDeployment := deployutil.LatestDeploymentInfo(config, existingDeployments)
Copy link
Contributor

Choose a reason for hiding this comment

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

this is more "latestControllerCreated" :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I made it to be consistent with some other occurrences in our code
yes, (latestRCCreated || latestRCRunning || latestRCDeployed) - I guess exists covers it but I don't really have a preference except we stop calling it latestDeployed which I find confusing because it implies it is in terminal state

@mfojtik
Copy link
Contributor

mfojtik commented Oct 24, 2017

/retest

@tnozicka
Copy link
Contributor Author

tnozicka commented Oct 26, 2017 •
edited

#17042 is in. Tests have a chance on passing now :)
/retest

@tnozicka tnozicka force-pushed the fix-multiple-deployer-pods-due-to-cancellation branch from e8cad12 to 6a1e162 Compare October 26, 2017 18:41
@tnozicka
Copy link
Contributor Author

rebased

@tnozicka
Copy link
Contributor Author

/retest

1 similar comment
@tnozicka
Copy link
Contributor Author

/retest

@tnozicka
Copy link
Contributor Author

@mfojtik tests are green

/test all

@tnozicka
Copy link
Contributor Author

/hold cancel

@openshift-ci-robot openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 31, 2017
@tnozicka
Copy link
Contributor Author

@mfojtik I think we should merge it if you are fine with it. This should be part of 3.7.

@tnozicka tnozicka added this to the 3.7.0 milestone Oct 31, 2017
@tnozicka tnozicka added kind/bug Categorizes issue or PR as related to a bug. area/tests labels Oct 31, 2017
@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 20, 2017
@stevekuznetsov
Copy link
Contributor

@mfojtik are we still targeting 3.7(.1?) with this?

@tnozicka tnozicka modified the milestones: 3.7.0, 3.8.0 Nov 28, 2017
@tnozicka tnozicka force-pushed the fix-multiple-deployer-pods-due-to-cancellation branch from 28b75c0 to ba495e5 Compare November 29, 2017 11:45
@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 29, 2017
@tnozicka
Copy link
Contributor Author

@mfojtik rebased and ready for your review

@mfojtik
Copy link
Contributor

mfojtik commented Nov 30, 2017

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Nov 30, 2017
@openshift-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mfojtik, tnozicka

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these OWNERS Files:

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 30, 2017
@openshift-merge-robot
Copy link
Contributor

/test all [submit-queue is verifying that this PR is safe to merge]

@openshift-merge-robot
Copy link
Contributor

Automatic merge from submit-queue (batch tested with PRs 17350, 16998).

openshift-merge-robot added a commit that referenced this pull request Nov 30, 2017
@openshift-merge-robot
Merge pull request #16998 from tnozicka/fix-multiple-deployer-pods-du…
6f2ba91 
…e-to-cancellation

Automatic merge from submit-queue (batch tested with PRs 17350, 16998).

apps: extend extended tests to better check for deployer invariants and enable back the old check

enabling back #16956
@openshift-merge-robot openshift-merge-robot merged commit 6f2ba91 into openshift:master Nov 30, 2017
@openshift-ci-robot
Copy link

openshift-ci-robot commented Nov 30, 2017 •
edited

@tnozicka: The following test failed, say /retest to rerun them all:

Test name Commit Details Rerun command
ci/openshift-jenkins/extended_conformance_install_update ba495e5 link /test extended_conformance_install_update

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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/test-infra repository. I understand the commands that are listed here.

@tnozicka tnozicka deleted the fix-multiple-deployer-pods-due-to-cancellation branch November 30, 2017 14:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mfojtik mfojtik mfojtik left review comments

Assignees

@mfojtik mfojtik

@tnozicka tnozicka

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/tests kind/bug Categorizes issue or PR as related to a bug. lgtm Indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
v3.8
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@tnozicka @mfojtik @stevekuznetsov @openshift-ci-robot @openshift-merge-robot @soltysh