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

Operator should not delete CRDs #172

Merged
merged 1 commit into from
Nov 9, 2023
Merged

Operator should not delete CRDs #172

merged 1 commit into from
Nov 9, 2023

Conversation

jkhelil
Copy link
Contributor

@jkhelil jkhelil commented Nov 6, 2023 •
edited by adambkaplan
Loading

Changes

  • Filter Crds before deleting resources because operator should not delete Crds when shipwright instance is deleted

Fixes #173

Submitter Checklist

  • Includes tests if functionality changed/was added
  • Includes docs if changes are user-facing
  • Set a kind label on this PR
  • Release notes block has been filled in, or marked NONE

See the contributor guide
for details on coding conventions, github and prow interactions, and the code review process.

Release Notes

Shipwright's Build CRDs are no longer deleted when the `ShipwrightBuild` object is deleted.

@pull-request-size pull-request-size bot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Nov 6, 2023
@jkhelil jkhelil changed the title Operator should not delete CRds Operator should not delete CRDs Nov 6, 2023
@adambkaplan
Copy link
Member

/kind bug

@openshift-ci openshift-ci bot added the kind/bug Categorizes issue or PR as related to a bug. label Nov 6, 2023
adambkaplan
adambkaplan requested changes Nov 6, 2023
View reviewed changes
Copy link
Member

@adambkaplan adambkaplan left a comment

Choose a reason for hiding this comment

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

/approve

Giving 👍 on this fix going in. I only ask that my comment on default_test.go be addressed before merging.

controllers/default_test.go
Comment on lines +247 to +250
test.CRDEventuallyExists(ctx, k8sClient, "builds.shipwright.io")
test.CRDEventuallyExists(ctx, k8sClient, "buildruns.shipwright.io")
test.CRDEventuallyExists(ctx, k8sClient, "buildstrategies.shipwright.io")
test.CRDEventuallyExists(ctx, k8sClient, "clusterbuildstrategies.shipwright.io")
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if we should introduce a wait period before checking that the CRDs still exist?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@adambkaplan The test is passing without the wait period, we do the same for the deployment and rbac and those tests had been working since the begining.
https://github.com/shipwright-io/operator/blob/main/controllers/default_test.go#L222

controllers/default_test.go
Copy link
Member

Choose a reason for hiding this comment

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

The AfterEach for this set of tests should delete the *.shipwright.io CRD instances. I think the tests previously relied on the delete behavior to tear down the CRD instance each time.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@adambkaplan Should we care about not deleteing the CRDs on tear down if the cluster is been stopped/deleed

@openshift-ci OpenShift CI
Copy link
Contributor

openshift-ci bot commented Nov 6, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: adambkaplan

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 6, 2023
@adambkaplan
Copy link
Member

/lgtm

I am going to merge this as-is. Any additional work regarding test setup/teardown can be addressed in a follow-up pull request.

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Nov 9, 2023
@openshift-merge-bot openshift-merge-bot bot merged commit ed38c79 into shipwright-io:main Nov 9, 2023
3 checks passed
@adambkaplan
Copy link
Member

Fixes #174

@adambkaplan adambkaplan added this to the release-v0.13.0 milestone Nov 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@adambkaplan adambkaplan adambkaplan requested changes

@qu1queee qu1queee Awaiting requested review from qu1queee

@SaschaSchwarze0 SaschaSchwarze0 Awaiting requested review from SaschaSchwarze0

Assignees

@adambkaplan adambkaplan

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. kind/bug Categorizes issue or PR as related to a bug. lgtm Indicates that a PR is ready to be merged. release-note size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
Shipwright Overview
Archived in project
Milestone
release-v0.13.0
Development

Successfully merging this pull request may close these issues.

[BUG] Operator RBAC Permissions are overly broad
2 participants
@jkhelil @adambkaplan