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

🌱 Add a make target and check in CI to verify CRD compatibility #1449

Merged
merged 4 commits into from
Nov 26, 2024
Merged

🌱 Add a make target and check in CI to verify CRD compatibility #1449

merged 4 commits into from
Nov 26, 2024

Conversation

everettraven
Copy link
Contributor

Description

  • Adds https://github.com/everettraven/crd-diff as a bingo dependency
  • Adds a make target called verify-crd-compatibility to run crd-diff and check for CRD incompatibilities
  • Adds a GitHub Actions Workflow to run make verify-crd-compatibility on Pull Requests

Reviewer Checklist

  • API Go Documentation
  • Tests: Unit Tests (and E2E Tests, if appropriate)
  • Comprehensive Commit Messages
  • Links to related GitHub Issue(s)

@everettraven everettraven requested a review from a team as a code owner November 13, 2024 00:28
@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 13, 2024
@netlify Netlify
Copy link

netlify bot commented Nov 13, 2024 •
edited
Loading

✅ Deploy Preview for olmv1 ready!

Name Link
🔨 Latest commit 0f27438
🔍 Latest deploy log https://app.netlify.com/sites/olmv1/deploys/6740dadd39e4d8000826ad98
😎 Deploy Preview https://deploy-preview-1449--olmv1.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@everettraven everettraven changed the title [WIP] 🌱 Add a barebones make target and check in CI to verify CRD compatibility 🌱 Add a barebones make target and check in CI to verify CRD compatibility Nov 13, 2024
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 13, 2024
@everettraven everettraven changed the title 🌱 Add a barebones make target and check in CI to verify CRD compatibility 🌱 Add a make target and check in CI to verify CRD compatibility Nov 13, 2024
everettraven
everettraven commented Nov 13, 2024
View reviewed changes
.bingo/Variables.mk Outdated
@@ -1,4 +1,4 @@
# Auto generated binary variables helper managed by https://github.com/bwplotka/bingo v0.9. DO NOT EDIT.
# Auto generated binary variables helper managed by https://github.com/bwplotka/bingo v0.8. DO NOT EDIT.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, I didn't realize I wasn't up to date on my bingo version. I don't think we should touch the auto-generated files though. If we are concerned about this particular change I can update my bingo version to v0.9

@everettraven
Copy link
Contributor Author

Closing and re-opening to pull in upgrade-e2e CI fixes in #1451

@everettraven everettraven reopened this Nov 13, 2024
bentito
bentito reviewed Nov 13, 2024
View reviewed changes
.bingo/Variables.mk Outdated Show resolved Hide resolved
@everettraven
Copy link
Contributor Author

the go-verdiff check looks like it is expected to fail in this scenario because this PR adds a go.mod file that didn't previously exist.

everettraven
everettraven commented Nov 25, 2024
View reviewed changes
crd-diff-config.yaml
enum:
enabled: true
removalEnforcement: "Strict"
additionEnforcement: "Strict"
Copy link
Contributor Author

@everettraven everettraven Nov 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

This enforcement means addition of new allowed enum values is considered incompatible. I know we have some areas of our API where we anticipate adding some enum values so we may want to move this to the less strict IfPreviouslyConstrained that allows addition of new allowed enum values only if the property was previously constrained by enum values. (i.e adding net new enum restrictions to a property is still considered incompatible)

I left this as strict as possible for now so that we can have a discussion as to whether we want to start as strict as possible and loosen as needed (where it makes sense) or leave room for planned future changes to not be "prevented" by this more restrictive configuration.

LalatenduMohanty
LalatenduMohanty approved these changes Nov 26, 2024
View reviewed changes
Copy link
Member

@LalatenduMohanty LalatenduMohanty left a comment

Choose a reason for hiding this comment

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

/lgtm

@everettraven everettraven added this pull request to the merge queue Nov 26, 2024
@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Nov 26, 2024
Merged via the queue into operator-framework:main with commit 2b4c083 Nov 26, 2024
18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tmshort tmshort tmshort left review comments

@camilamacedo86 camilamacedo86 camilamacedo86 left review comments

@bentito bentito bentito left review comments

@LalatenduMohanty LalatenduMohanty LalatenduMohanty approved these changes

Assignees

@LalatenduMohanty LalatenduMohanty

Labels
lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@everettraven @LalatenduMohanty @bentito @camilamacedo86 @tmshort