Skip to content

Conversation

awgreene
Copy link
Member

@awgreene awgreene commented Nov 9, 2020

This commit introduced the OperatorCondition CRD which allows operators
to communicate advanced state directly with OLM.

// +kubebuilder:storageversion
// +kubebuilder:subresource:status
// OperatorCondition is a Custom Resource of type `OperatorCondition` which is used to convey information to OLM about the state of an operator.
type OperatorCondition struct {
Copy link
Member Author

Choose a reason for hiding this comment

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

Condition struct was already defined.

Copy link

@jmccormick2001 jmccormick2001 left a comment

Choose a reason for hiding this comment

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

is the user facing documentation on how to use this located in another PR? or is it yet to be written?

@anik120
Copy link
Contributor

anik120 commented Nov 9, 2020

is the user facing documentation on how to use this located in another PR? or is it yet to be written?

We should probably document this here and link that PR here so that we can merge both of them together/at least one right after the other.


// OperatorConditionSpec allows a cluster admin to convey information about the state of an operator to OLM, potentially overriding state reported by the operator.
type OperatorConditionSpec struct {
Overrides []Condition `json:"overrides,omitempty"`
Copy link
Member

Choose a reason for hiding this comment

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

Should we add a TODO on the Condition type to migrate to metav1.Condition when we move everything to k8s 1.19+?

Is there anything that would prevent us from doing that? (e.g. a breaking change)

Copy link
Member Author

Choose a reason for hiding this comment

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

We could use the metav1.Condition but we should be recognize that OLM's condition type has different fields:

  • OLM's Condition has the LastUpdateTime field.
  • The metav1.Condition has the ObservedGeneration field.

If we plan to update this struct to use the metav1.Condition we will have to handle any CRs that have set the LastUpdateTime field. Thoughts?

Copy link
Member

Choose a reason for hiding this comment

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

I would definitely opt for using metav1.Condition directly. We should diverge as little as possible. If we find that LastUpdateTime is actually useful, then we can create a composite struct that embeds metav1.Condition.

Copy link
Member

Choose a reason for hiding this comment

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

to add to this, I was planning on replacing the Condition struct you're using w/ metav1.Condition at some point down the road as well.

@awgreene awgreene force-pushed the condition-crd branch 2 times, most recently from accde41 to 40a9220 Compare November 10, 2020 21:07
@awgreene awgreene force-pushed the condition-crd branch 3 times, most recently from 2fc06a7 to a8a46e2 Compare November 12, 2020 17:15
Copy link
Member

@dinhxuanvu dinhxuanvu left a comment

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Nov 12, 2020
This commit introduces the OperatorCondition CRD which is used by an
operator to communicate state to OLM.
@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Nov 12, 2020
Copy link
Member

@dinhxuanvu dinhxuanvu left a comment

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Nov 12, 2020
@kevinrizza
Copy link
Member

/approve

Copy link
Member

@joelanford joelanford left a comment

Choose a reason for hiding this comment

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

/lgtm

@awgreene awgreene merged commit 820e285 into operator-framework:master Nov 12, 2020
@jianzhangbjz
Copy link
Contributor

/QE-approved

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

lgtm Indicates that a PR is ready to be merged.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

10 participants