-
Notifications
You must be signed in to change notification settings - Fork 71
Strimzi v1
CRD API and 1.0.0 release
#174
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
Strimzi v1
CRD API and 1.0.0 release
#174
Conversation
replicas
field required everywhere?
strimzi/strimzi-kafka-operator#11809
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.
@scholzj thanks for this proposal.
I have a few comments.
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.
Transition steps and required changes are outlined clearly.
Bump to 1.0.0 makes sense on the back of the transition to v1, but that would be subject to maintainer consensus
I left a few minor comments
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, thanks.
With the exception of the `KafkaUser` resource, users are not expected to do anything in the first phase other than upgrade the CRDs and the other Strimzi resources. | ||
|
||
The `Kafka` CRD YAML with both `v1beta2` and `v1` versions is too big and it would not be possible to use it with `kubectl apply` because of its size. | ||
Instead of updating all our docs and forcing users to use `kubectl creat` / `kubectl replace` instead of `kubectl apply`, we will generate the `Kafka` CRD with the `description` fields. |
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.
I am confused now, because you commented:
But basically, if the CRD is too big, kubectl apply will nto work for it. So we make it smaller by leaving out the API descriptions while having both v1beta2 and v1 versions.
So AFAIU "without the description fields" while here I read "with the description fields".
Also, isn't it good having descriptions in the CRD? Isn't it something you can get via the kubectl describe
instead of going to the doc?
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.
That is a typo, yes. It should be without
.
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.
Also, isn't it good having descriptions in the CRD? Isn't it something you can get via the kubectl describe instead of going to the doc?
Well, it is not perfect. That is why I mention it in the proposal. But I believe it is still better to not have the descriptions than have to:
- Update all our docs to cover the whole
kubectl create
/kubectl replace
process - Handle all the questions from users who will still use
kubectl apply
and it would fail for them - Handle all the questions from the users who did not even noticed
kubectl apply
failed for them
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.
While removing descriptions helps now, it could just delay the problem later if the CRDs increase. Anyway I have not strong opinion on it. Curious to know what other @strimzi/maintainers think about it.
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.
So, it depends on what later do you really mean. Keep in mind that:
- The transition to
v1
requires both versions in the CRD. Once we dropv1beta2
it will be small again - The
v1
drops all kinds of options around ZooKeeper, so it is smaller than thev1beta2
- We also need to think a bit more about the APIs we add. I think Deprecate and remove the
type: oauth
andtype: keycloak
APIs #175 is an example of where it grew up huge without much added value compared to having just some unstructuredconfig
map. So we should try to take these lessons and apply them, for example, here Proposal for supportingconnections.max.reauth.ms
configuration for SCRAM listeners #173
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.
This all makes sense to me and I'm happy with the proposed changes. Only had a couple of nits and I wonder if it's worth stating anywhere that since the Access Operator CRDs are versioned separately (it's currently using the API version v1alpha1
) the API version for that is remaining unchanged.
With the exception of the `KafkaUser` resource, users are not expected to do anything in the first phase other than upgrade the CRDs and the other Strimzi resources. | ||
|
||
The `Kafka` CRD YAML with both `v1beta2` and `v1` versions is too big and it would not be possible to use it with `kubectl apply` because of its size. | ||
Instead of updating all our docs and forcing users to use `kubectl creat` / `kubectl replace` instead of `kubectl apply`, we will generate the `Kafka` CRD without the `description` fields. |
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.
Instead of updating all our docs and forcing users to use `kubectl creat` / `kubectl replace` instead of `kubectl apply`, we will generate the `Kafka` CRD without the `description` fields. | |
Instead of updating all our docs and forcing users to use `kubectl create` / `kubectl replace` instead of `kubectl apply`, we will generate the `Kafka` CRD without the `description` fields. |
Will this effect the generation of the docs? I personally use CRD yaml files and the description
fields to check what a field is used for. Would it be possible to include the description
field for the v1
API but not v1beta2
?
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.
Will this effect the generation of the docs?
No, the docs will not be affected.
Would it be possible to include the description field for the v1 API but not v1beta2?
Including the description for at least one version is unfortunately not possible. Both are needed to get the file under the size limit.
I added both to the proposal.
I added an |
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.
Thanks for the proposal, just few comments.
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.
Signed-off-by: Jakub Scholz <[email protected]>
Co-authored-by: PaulRMellor <[email protected]> Signed-off-by: Jakub Scholz <[email protected]>
Signed-off-by: Jakub Scholz <[email protected]>
Signed-off-by: Jakub Scholz <[email protected]>
Signed-off-by: Jakub Scholz <[email protected]>
Signed-off-by: Jakub Scholz <[email protected]>
Signed-off-by: Jakub Scholz <[email protected]>
Signed-off-by: Jakub Scholz <[email protected]>
Signed-off-by: Jakub Scholz <[email protected]>
Signed-off-by: Jakub Scholz <[email protected]>
Signed-off-by: Jakub Scholz <[email protected]>
Signed-off-by: Jakub Scholz <[email protected]>
c7f8e86
to
f5aabff
Compare
This has now 4 binding and 2 non-binding +1 votes. If there are no more comments, I will close it as approved on Thursday. |
Signed-off-by: Jakub Scholz <[email protected]>
Signed-off-by: Jakub Scholz <[email protected]>
Approved with 4 binding and 2 non-binding +1 votes |
* Strimzi v1 CRD API and 1.0.0 release Signed-off-by: Jakub Scholz <[email protected]> * Apply suggestions from code review Co-authored-by: PaulRMellor <[email protected]> Signed-off-by: Jakub Scholz <[email protected]> * Review comments FV anf PM Signed-off-by: Jakub Scholz <[email protected]> * Review comments PP Signed-off-by: Jakub Scholz <[email protected]> * Add link to the Connect proposal Signed-off-by: Jakub Scholz <[email protected]> * with -> without Signed-off-by: Jakub Scholz <[email protected]> * Review comemnts KS Signed-off-by: Jakub Scholz <[email protected]> * Review comments KS Signed-off-by: Jakub Scholz <[email protected]> * Update link to MM2 proposal Signed-off-by: Jakub Scholz <[email protected]> * Review comments TS Signed-off-by: Jakub Scholz <[email protected]> * Mention additionalKanikoOptions Signed-off-by: Jakub Scholz <[email protected]> * Update after OAuth deprecation merged Signed-off-by: Jakub Scholz <[email protected]> * Remove enableECDSA as the whole OAuth is gone Signed-off-by: Jakub Scholz <[email protected]> * Update index Signed-off-by: Jakub Scholz <[email protected]> --------- Signed-off-by: Jakub Scholz <[email protected]> Co-authored-by: PaulRMellor <[email protected]>
This PR opens the proposal for how we should deal with the
v1
CRD API, its introduction, removal of the older API versions, and finally the release of Strimzi 1.0.0. While there are some open points left, they can be done in a separate follow-up proposals. So please do not hesitate to review it 😉.