Skip to content

Annotate .status.conditions as map-list #226

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

Merged
merged 5 commits into from
Oct 13, 2021
Merged

Conversation

nightkr
Copy link
Member

@nightkr nightkr commented Oct 5, 2021

Description

This allows server-side apply to update conditions individually. Sadly, it must be applied manually for each CRD crate to take effect.

Requires #225 in order for operators not to misuse SSA to trample all over each others' conditions anyway.

Review Checklist

  • Code contains useful comments
  • (Integration-)Test cases added (or not applicable)
  • Documentation added (or not applicable)
  • Changelog updated (or not applicable)

This allows server-side apply to update conditions individually. Sadly, it must
be applied manually for each CRD to take effect.
@nightkr nightkr marked this pull request as ready for review October 5, 2021 10:10
@nightkr nightkr requested a review from a team October 5, 2021 10:10
nightkr added a commit to stackabletech/zookeeper-operator that referenced this pull request Oct 5, 2021
@lfrancke
Copy link
Member

lfrancke commented Oct 5, 2021

Only on my phone: We used to have this and removed it when k8s-openapi introduced the schemas. Does this mean this should really be upstream?

@nightkr
Copy link
Member Author

nightkr commented Oct 5, 2021

I'll look into it, but sadly I don't /think/ schemars allows this kind of customization on the type level without opting out of the derive macro completely.

@nightkr
Copy link
Member Author

nightkr commented Oct 5, 2021

Yep.. GREsau/schemars#50

@lfrancke
Copy link
Member

lfrancke commented Oct 5, 2021

Okay, good point. Thanks.
Watching that issue now.
We had a "manual" schema for one other thing that we removed....can't remember which... LabelSelector maybe?

@nightkr
Copy link
Member Author

nightkr commented Oct 6, 2021

Okay, so this isn't actually required anyway it seems, since k8s-openapi does its own schema codegen without relying on the derive...

@nightkr
Copy link
Member Author

nightkr commented Oct 6, 2021

Oh right, we can't even override that generically anyway, since we don't have any generic way to control Vec<T>'s schema (even if we do control T's).

The only way to get around that would be to define our own Conditions wrapper type (or a generic OrderedMap, I suppose).

lfrancke
lfrancke previously approved these changes Oct 8, 2021
@soenkeliebau soenkeliebau merged commit 7965121 into main Oct 13, 2021
@soenkeliebau soenkeliebau deleted the bugfix/conditions-schema branch October 13, 2021 13:06
nightkr added a commit to stackabletech/zookeeper-operator that referenced this pull request Oct 18, 2021
nightkr added a commit to stackabletech/zookeeper-operator that referenced this pull request Oct 19, 2021
* Fix .status.conditions schema

Depends on stackabletech/operator-rs#226, merge that
before this.

* Add to changelog

* Switch back to operator-rs#main

Because stackabletech/operator-rs#226 was merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants