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

Fix a bug in deletion of webhook service for replacement #2606

Merged
merged 1 commit into from
Feb 9, 2022
Merged

Fix a bug in deletion of webhook service for replacement #2606

merged 1 commit into from
Feb 9, 2022

Conversation

orenc1
Copy link
Contributor

@orenc1 orenc1 commented Feb 1, 2022 •
edited by timflannagan
Loading

The condition that checks for an error with the webhook service deletion is based on the wrong variable, and the return statement is never reached, which leads to a continuous error when OLM tries to replace the webhook service on certain conditions

Signed-off-by: orenc1 [email protected]

Description of the change:

Motivation for the change:

https://bugzilla.redhat.com/show_bug.cgi?id=2048441

Reviewer Checklist

  • Implementation matches the proposed design, or proposal is updated to match implementation
  • Sufficient unit test coverage
  • Sufficient end-to-end test coverage
  • Docs updated or added to /doc
  • Commit messages sensible and descriptive

@openshift-ci
Copy link

openshift-ci bot commented Feb 1, 2022

Hi @orenc1. Thanks for your PR.

I'm waiting for a operator-framework 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/test-infra repository.

@openshift-ci openshift-ci bot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Feb 1, 2022
@timflannagan timflannagan changed the title [BZ #2048441] Fix a bug in deletion of webhook service for replacement Fix a bug in deletion of webhook service for replacement Feb 1, 2022
@timflannagan
Copy link
Member

(Removing the BZ from the title as it messes up downstream automation)

/ok-to-test

@openshift-ci openshift-ci bot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Feb 1, 2022
@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 1, 2022
@orenc1
Copy link
Contributor Author

orenc1 commented Feb 1, 2022

looks like the e2e failures were transient.
/retest

dinhxuanvu
dinhxuanvu approved these changes Feb 1, 2022
View reviewed changes
Copy link
Member

@dinhxuanvu dinhxuanvu left a comment

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Feb 1, 2022
awgreene
awgreene approved these changes Feb 1, 2022
View reviewed changes
Copy link
Member

@awgreene awgreene left a comment

Choose a reason for hiding this comment

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

Thanks for submitting this fix @orenc1

@orenc1
Copy link
Contributor Author

orenc1 commented Feb 1, 2022

/retest

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@orenc1
Copy link
Contributor Author

orenc1 commented Feb 1, 2022

/retest-required

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

3 similar comments
@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@orenc1
Copy link
Contributor Author

orenc1 commented Feb 2, 2022

/retest-required

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

1 similar comment
@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@orenc1
Copy link
Contributor Author

orenc1 commented Feb 2, 2022

/retest

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@orenc1
Copy link
Contributor Author

orenc1 commented Feb 2, 2022

/retest

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Feb 2, 2022
@awgreene
Copy link
Member

awgreene commented Feb 3, 2022

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Feb 3, 2022
@awgreene awgreene closed this Feb 3, 2022
@awgreene awgreene reopened this Feb 3, 2022
@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@awgreene awgreene force-pushed the fix_delete_service_bug branch from 059b9ba to 539cc69 Compare February 3, 2022 20:51
@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Feb 3, 2022
@orenc1
Copy link
Contributor Author

orenc1 commented Feb 4, 2022

/retest

@orenc1
[BZ #2048441] Fix a bug in deletion of webhook service for replacement
67c533f 
The condition that checks for an error with the webhook service deletion is based on the wrong variable, and the return statement is never reached, which leads to a continuous error when OLM tries to replace the webhook service on certain conditions

Signed-off-by: orenc1 <[email protected]>
@orenc1 orenc1 force-pushed the fix_delete_service_bug branch from 539cc69 to 67c533f Compare February 4, 2022 20:13
@orenc1
Copy link
Contributor Author

orenc1 commented Feb 8, 2022

hi @awgreene do you think we are ready to rerun the CI?
thanks

perdasilva
perdasilva reviewed Feb 9, 2022
View reviewed changes
pkg/controller/install/certresources.go
@@ -246,7 +246,7 @@ func (i *StrategyDeploymentInstaller) installCertRequirementsForDeployment(deplo

// Delete the Service to replace
deleteErr := i.strategyClient.GetOpClient().DeleteService(service.GetNamespace(), service.GetName(), &metav1.DeleteOptions{})
if err != nil && !k8serrors.IsNotFound(deleteErr) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This looks like something that should have been picked up by unit testing. Would it be a lot of effort to add a test?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Since e2e is green, let's merge and put the unit test in a follow up PR. Would that work for you?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, although I'm not sure how can we check this particular case in a unit test, in which the service could not be deleted and therefore the installCertRequirementsForDeployment method returns an error.
I assume we'll need to use:

mockOpClient.EXPECT().DeleteService(namespace, "test-service", &metav1.DeleteOptions{}).Return(fmt.Errorf("could not delete existing service test-service"))

But not quite sure how to proceed.

@openshift-ci
Copy link

openshift-ci bot commented Feb 9, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: awgreene, dinhxuanvu, orenc1, timflannagan

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 [awgreene,dinhxuanvu,timflannagan]

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

@timflannagan
Copy link
Member

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Feb 9, 2022
@openshift-merge-robot openshift-merge-robot merged commit e9aef37 into operator-framework:master Feb 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@perdasilva perdasilva perdasilva left review comments

@awgreene awgreene awgreene approved these changes

@dinhxuanvu dinhxuanvu dinhxuanvu approved these changes

@timflannagan timflannagan timflannagan approved these changes

@ecordell ecordell Awaiting requested review from ecordell

@gallettilance gallettilance Awaiting requested review from gallettilance

Assignees

@awgreene awgreene

@dinhxuanvu dinhxuanvu

@timflannagan timflannagan

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. ok-to-test Indicates a non-member PR verified by an org member that is safe to test.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@orenc1 @timflannagan @openshift-bot @awgreene @perdasilva @dinhxuanvu @openshift-merge-robot