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

Resolve Install Plan before Requiring Approval #364

Merged
merged 2 commits into from
Jun 7, 2018

Conversation

alecmerdler
Copy link
Member

@alecmerdler alecmerdler commented Jun 7, 2018

Description

Fixes regression introduced which prevented status.plan from being filled before a manual install plan is approved. Also updates docs to include the RequiresApproval phase.

Addresses https://jira.coreos.com/browse/ALM-623

@alecmerdler alecmerdler requested a review from ecordell June 7, 2018 16:44
@@ -54,7 +54,6 @@ type InstallPlanConditionType string

const (
InstallPlanResolved InstallPlanConditionType = "Resolved"
InstallPlanApproved InstallPlanConditionType = "Approved"
Copy link
Member Author

Choose a reason for hiding this comment

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

Unused, should be determined from spec.approved.

@alecmerdler
Copy link
Member Author

alecmerdler commented Jun 7, 2018

The e2e tests were failing because of the server could not find the requested resource when trying to fetch the resources in status.plan, which is what we want because the InstallPlan is not approved. This test was probably passing before because status.plan was empty due to the regression, so we never tried to fetch anything.

@alecmerdler alecmerdler force-pushed the ALM-623 branch 2 times, most recently from 9daa86c to ee46f52 Compare June 7, 2018 17:36
@@ -132,21 +132,22 @@ func TestCreateInstallPlanManualApproval(t *testing.T) {
if step.Resource.Kind == "CustomResourceDefinition" {
_, err := c.GetCustomResourceDefinition(step.Resource.Name)

require.NoError(t, err)
require.Error(t, err)
vaultResourcesPresent = vaultResourcesPresent + 1
Copy link
Member Author

Choose a reason for hiding this comment

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

@ecordell This fails because our CI target cluster always has all the CRDs. Should we just disable this check?

@ecordell
Copy link
Member

ecordell commented Jun 7, 2018

@alecmerdler For the e2e tests, we want them to be aware of the approval state:

  1. if plan is resolved but not approved, verify that the resources in the plan haven't been created (as you pointed out we won't be able to do that for the CRDs)
  2. if the plan is resolved and approved, verify that all of the resources in the plan exist

Copy link
Member

@ecordell ecordell left a comment

Choose a reason for hiding this comment

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

LGTM

@alecmerdler alecmerdler merged commit 98ec036 into operator-framework:master Jun 7, 2018
@alecmerdler alecmerdler deleted the ALM-623 branch June 7, 2018 19:43
njhale pushed a commit to njhale/operator-lifecycle-manager that referenced this pull request Sep 10, 2018
Resolve Install Plan before Requiring Approval
ecordell pushed a commit to ecordell/operator-lifecycle-manager that referenced this pull request Mar 8, 2019
Resolve Install Plan before Requiring Approval
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.

2 participants