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.

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

Add missing tags to []conditions which got reported by crd checker #584

Closed
wants to merge 1 commit into from

Conversation

stuggi
Copy link
Contributor

@stuggi stuggi commented Dec 3, 2024

Adds the task which got reported by the crd checker recently added as a pre-commit validation.

Also syncs the validation of the conditon parameters with [1].

In a follow up, we should integrate the new ObservedGeneration for a condition [1]. The observedGeneration represents the .metadata.generation that the condition was set based upon.

[1] https://github.com/kubernetes/apimachinery/blob/release-1.29/pkg/apis/meta/v1/types.go#L1497
[1] https://github.com/kubernetes/apimachinery/blob/release-1.29/pkg/apis/meta/v1/types.go#L1518

Adds the task which got reported by the crd checker recently added
as a pre-commit validation.

Also syncs the validation of the conditon parameters with [1].

In a follow up, we should integrate the new ObservedGeneration for
a condition [1]. The observedGeneration represents the
.metadata.generation that the condition was set based upon.

[1] https://github.com/kubernetes/apimachinery/blob/release-1.29/pkg/apis/meta/v1/types.go#L1497
[1] https://github.com/kubernetes/apimachinery/blob/release-1.29/pkg/apis/meta/v1/types.go#L1518

Signed-off-by: Martin Schuppert <mschuppert@redhat.com>
@stuggi
Copy link
Contributor Author

stuggi commented Dec 3, 2024

an alternative for minimal required changes would be #585

@stuggi stuggi requested a review from dprince December 3, 2024 12:47
Type Type `json:"type"`

// Status of the condition, one of True, False, Unknown.
// status of the condition, one of True, False, Unknown.
// +required
Copy link
Contributor

@bogdando bogdando Dec 3, 2024

Choose a reason for hiding this comment

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

as Conditions got included into CRDs, we cannot change optional fields to become required, do we?
Also, how that change passes the crd checker? Let's try to include this into some operator as a dependency and see if crd checker allows that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it is already required.

              conditions:
                description: Conditions
                items:
                  description: Condition defines an observation of a API resource
                    operational state.
                  properties:
                    lastTransitionTime:
                      description: Last time the condition transitioned from one status
                        to another. This should be when the underlying condition changed.
                        If that is not known, then using the time when the API field
                        changed is acceptable.
                      format: date-time
                      type: string
                    message:
                      description: A human readable message indicating details about
                        the transition.
                      type: string
                    reason:
                      description: The reason for the condition's last transition
                        in CamelCase.
                      type: string
                    severity:
                      description: Severity provides a classification of Reason code,
                        so the current situation is immediately understandable and
                        could act accordingly. It is meant for situations where Status=False
                        and it should be indicated if it is just informational, warning
                        (next reconciliation might fix it) or an error (e.g. DB create
                        issue and no actions to automatically resolve the issue can/should
                        be done). For conditions where Status=Unknown or Status=True
                        the Severity should be SeverityNone.
                      type: string
                    status:
                      description: Status of the condition, one of True, False, Unknown.
                      type: string
                    type:
                      description: Type of condition in CamelCase.
                      type: string
                  required:
                  - lastTransitionTime
                  - status
                  - type
                  type: object
                type: array

but we probably go just with #585 for now.

@stuggi
Copy link
Contributor Author

stuggi commented Dec 3, 2024

closing in favor of #585

@stuggi stuggi closed this Dec 3, 2024
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.

2 participants