-
Notifications
You must be signed in to change notification settings - Fork 65
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
BREAKING: chore: align BundleCondition with upstream metav1.Condition #249
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
/approve
/hold
This looks like an improvement to me. A lot of these changes are technically breaking if we nitpick, but I suspect that it'd be worthwhile to make the changes anyway - I don't feel like we need to wait for v1alpha2
or v1beta1
for these changes.
The hold is either for us to change this PR to be non-breaking (which I absolutely don't expect us to do) or to add an indidator to the PR description that it might be breaking (and thus would be worth mentioning in release notes)
deploy/charts/trust-manager/templates/trust.cert-manager.io_bundles.yaml
Show resolved
Hide resolved
33735b9
to
7480444
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
/approve
Thanks for this 😁 Took a bit of discussion to get here but I think this is a worthwhile improvement!
Signed-off-by: Erik Godding Boye <egboye@gmail.com>
7480444
to
d603faf
Compare
/test pull-trust-manager-verify |
/lgtm Thanks! |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: SgtCoDFish 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 |
This is a PR that replaces #240. Since we decided to not migrate to the upstream
metav1.Condition
, I have tried to align our ownBundleCondition
as much as possible the the current upstream struct. Personally, I think it's an improvement that first and foremost will add more API spec validations better protecting us from controller bugs. This is technically breaking changes, but since the trust-manager controller should be the only actor modifyingBundle
status, I think it's fine.Also removing the obsolete validation in the webhook for duplicate conditions. This was code is not needed after #173.