search

cert-manager / trust-manager

trust-manager is an operator for distributing trust bundles across a Kubernetes cluster.
https://cert-manager.io/docs/projects/trust-manager/
Apache License 2.0
244 stars 66 forks source link

chore!: remove .status.target field from Bundle API #230

Closed erikgb closed 10 months ago

erikgb commented 10 months ago

... to simplify Bundle reconciler after migration to SSA. The now complex nested structure, inherited from .spec.target is almost not in effective use at present. I think maintaining a copy of the target field made sense before we migrated to SSA, since we had to do reconciliation in two steps, ref. https://github.com/cert-manager/trust-manager/issues/100. Many new features have also been added, making the BundletTarget struct grow.

Yes, I know this is a breaking change, but we are still on v1lpha1 version of the API. Also the API will be broken in under status (observed state), and I don't think many users will be affected by it.

erikgb commented 10 months ago

/retest

erikgb commented 10 months ago

/retest

SgtCoDFish commented 10 months ago

(I'm on holiday today so I probably won't be able to respond today beyond this comment)

As you can probably guess, I'm nervous about making any breaking change here - even for v1alpha1 😂

I spoke to a bunch of people at KubeCon who wanted to use more trust-manager but were worried about production-readiness. #228 was sparked by those discussions. I'm of the opinion that trust-manager is ready for prod and I want to encourage that as much as possible.

One of the things that was suggested at KubeCon was to create v1beta1 soon. I'd be quite comfortable with making this change in the migration to v1beta1. Would it be fine to create v1beta1 in the near future and leave v1alpha1 unbroken?

If we have to change v1alpha1, would it be possible to make .status.target an empty struct or a static dummy or something? Or is that going to be too cumbersome?

I'm not saying "no" to this PR, just exploring the alternatives. I recognise what you're aiming for I think!

erikgb commented 10 months ago

As you can probably guess, I'm nervous about making any breaking change here - even for v1alpha1 😂

In the spec part, I would expect that. But this is the status part - which I don't think many users really use. 😉 And if we look to the upstream API versioning practice, I think we are "convered": https://kubernetes.io/docs/reference/using-api/#api-versioning

I spoke to a bunch of people at KubeCon who wanted to use more trust-manager but were worried about production-readiness. #228 was sparked by those discussions. I'm of the opinion that trust-manager is ready for prod and I want to encourage that as much as possible.

Cool! I agree it's approaching production readiness - even if we use it in production without any issues already.

One of the things that was suggested at KubeCon was to create v1beta1 soon. I'd be quite comfortable with making this change in the migration to v1beta1. Would it be fine to create v1beta1 in the near future and leave v1alpha1 unbroken?

By adding a beta version, we send a signal about a more stable API. What if we create v1beta1, but make it unserved for now? I think we should improve the API more before approaching GA.

If we have to change v1alpha1, would it be possible to make .status.target an empty struct or a static dummy or something? Or is that going to be too cumbersome?

Why? We can drop using .status.target in the controller, but that's still breaking - but in a more sophisticated manner. 😆

I'm not saying "no" to this PR, just exploring the alternatives. I recognise what you're aiming for I think!

My goal is to simplify things. I am a big fan of simplicity.

jetstack-bot commented 10 months ago

[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: - ~~[OWNERS](https://github.com/cert-manager/trust-manager/blob/main/OWNERS)~~ [SgtCoDFish] Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment
erikgb commented 10 months ago

/retest