Skip to content

Include GetTopLevelConditionType for KRShaped#1298

Merged
knative-prow-robot merged 2 commits intoknative:masterfrom
whaught:conditiontype
May 5, 2020
Merged

Include GetTopLevelConditionType for KRShaped#1298
knative-prow-robot merged 2 commits intoknative:masterfrom
whaught:conditiontype

Conversation

@whaught
Copy link
Copy Markdown
Contributor

@whaught whaught commented May 5, 2020

Issue #1226

  • Each type will report its expected happy condition
  • KRShaped will detect its happy type by looking at its own conditions.
  • Added a unit test

At the moment each implementer either makes a LivingConditionSet manager or a BatchConditionSet manager within each resource. However this allows for polymorphic logic since each type will report its own happy condition.

Longer term we could simplify by just having GetConditionSet(resource) and the manager can figure out what the condition ought to be. This will make it easier to share common logic.

@googlebot googlebot added the cla: yes Indicates the PR's author has signed the CLA. label May 5, 2020
@knative-prow-robot knative-prow-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label May 5, 2020
Comment thread apis/duck/v1/kresource_type.go Outdated
func (t *KResource) GetHappyConditionType() apis.ConditionType {
// Note: KResources are unmarshalled from existing resources. This will only work properly for resources that
// have already been initialized to their type.
if cond := t.Status.GetCondition(apis.ConditionSucceeded); cond != nil {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

this will not work for un-initialized resources, the condition set will be an empty list.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

sorry I did not see the final impl, this will work but it wil also require all downstreams to update

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I have that included as a comment. The idea is that each API shape reports its type and gets initialized. KResource is a generic struct we unmarshal to, but not something that directly manages init.

Comment thread apis/duck/v1/kresource_type_test.go
Comment thread apis/test/example/v1alpha1/fiz_types.go
@knative-metrics-robot
Copy link
Copy Markdown

The following is the coverage report on the affected files.
Say /test pull-knative-pkg-go-coverage to re-run this coverage report

File Old Coverage New Coverage Delta
apis/duck/v1/kresource_type.go Do not exist 87.5%

@whaught whaught changed the title Include GetHappyConditionType for KRShaped Include GetTopLevelConditionType for KRShaped May 5, 2020
@n3wscott
Copy link
Copy Markdown
Contributor

n3wscott commented May 5, 2020

/lgtm
/approve

@knative-prow-robot knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label May 5, 2020
@knative-prow-robot
Copy link
Copy Markdown
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: n3wscott, whaught

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@knative-prow-robot knative-prow-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 5, 2020
@knative-prow-robot knative-prow-robot merged commit 1099bd1 into knative:master May 5, 2020

GetStatus() *Status

GetTopLevelConditionType() apis.ConditionType
Copy link
Copy Markdown
Contributor

@antoineco antoineco May 6, 2020

Choose a reason for hiding this comment

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

Too late, but I'm curious why we didn't stick to the current naming here (Happy instead of TopLevel).

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

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. cla: yes Indicates the PR's author has signed the CLA. lgtm Indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants