Skip to content
This repository has been archived by the owner on Jun 4, 2021. It is now read-only.

Embed a common Status duck type. #258

Merged
merged 16 commits into from
Mar 18, 2019
Merged

Embed a common Status duck type. #258

merged 16 commits into from
Mar 18, 2019

Conversation

n3wscott
Copy link
Contributor

@n3wscott n3wscott commented Mar 18, 2019 •
edited
Loading

Following knative/serving#3444

This embeds duckv1alpha1.Status (knative/pkg#324) in
all of our resource's status in place of having each independently define
ObservedGeneration and Conditions. The point of doing this is that Conditions
alone are insufficient to truly assess readiness because we also must make sure
that the generation that those conditions reflect (the observed generation)
matches the current generation of the resource (the metadata generation).

Also had to update the e2e test from pkg changes.

Release Note

Updates Serving to 0.4.1
Upgrade to Kubernetes 1.12.6.
Added ObservedGeneration to all Source CRDs.

combines #257

@knative-prow-robot knative-prow-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Mar 18, 2019
@n3wscott
Copy link
Contributor Author

/assign @vaikas-google
/cc @mattmoor

@n3wscott n3wscott changed the title Embed a common Status duck type. [WIP] Embed a common Status duck type. Mar 18, 2019
@knative-prow-robot knative-prow-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 18, 2019
grantr
grantr reviewed Mar 18, 2019
View reviewed changes
Gopkg.toml Outdated Show resolved Hide resolved
contrib/camel/pkg/apis/sources/v1alpha1/camelsource_types.go
// +patchMergeKey=type
// +patchStrategy=merge
Conditions duckv1alpha1.Conditions `json:"conditions,omitempty" patchStrategy:"merge" patchMergeKey:"type"`
duckv1alpha1.Status `json:",inline"`
Copy link
Contributor

Choose a reason for hiding this comment

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

See knative/eventing#913 (comment). This needs documentation about what fields are embedded here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think this comment will stay up to date for long. I am pushing back.

Copy link
Contributor

Choose a reason for hiding this comment

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

That gives me pause.

This embed is published API that users and integrators see and work with and build knowledge and products around. If we rename ObservedGeneration to SeenGeneration, that's a breaking API change, and it should be documented.

If duckv1alpha1.Status is going to be a shared interface, it can't change very often, and when it does, that change should be announced in every project that uses it.

@n3wscott n3wscott changed the title [WIP] Embed a common Status duck type. Embed a common Status duck type. Mar 18, 2019
@knative-prow-robot knative-prow-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 18, 2019
@evankanderson
Copy link
Member

/assign grantr

@n3wscott n3wscott mentioned this pull request Mar 18, 2019
Merged
@n3wscott
Copy link
Contributor Author

/hold

wait for #258

@knative-prow-robot knative-prow-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 18, 2019
grantr
grantr approved these changes Mar 18, 2019
View reviewed changes
Copy link
Contributor

@grantr grantr left a comment

Choose a reason for hiding this comment

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

Thanks @n3wscott!
/lgtm

Can you add something like this to the PR release note?

Upgrade to Kubernetes 1.12.6.
Added ObservedGeneration to all Source CRDs.

Gopkg.toml

[[override]]
name = "k8s.io/api"
version = "kubernetes-1.12.5"
version = "kubernetes-1.12.6"
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd probably mention this in release notes for completeness, but I don't think it's required.

contrib/camel/pkg/apis/sources/v1alpha1/camelsource_types.go
Conditions duckv1alpha1.Conditions `json:"conditions,omitempty" patchStrategy:"merge" patchMergeKey:"type"`
// inherits duck/v1alpha1 Status, which currently provides:
// * ObservedGeneration - the 'Generation' of the Service that was last processed by the controller.
// * Conditions - the latest available observations of a resource's current state.
Copy link
Contributor

Choose a reason for hiding this comment

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

✨

@knative-prow-robot knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Mar 18, 2019
@knative-prow-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

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

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

The pull request process is described here

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 Mar 18, 2019
@knative-prow-robot knative-prow-robot removed the lgtm Indicates that a PR is ready to be merged. label Mar 18, 2019
@n3wscott
Copy link
Contributor Author

/hold cancel

pkg docs updated.

@knative-prow-robot knative-prow-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 18, 2019
@grantr
Copy link
Contributor

grantr commented Mar 18, 2019

Looks like you need to rerun codegen, which is odd because you just did that

@grantr
Copy link
Contributor

grantr commented Mar 18, 2019

/lgtm

@knative-prow-robot knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Mar 18, 2019
@knative-prow-robot knative-prow-robot merged commit 7417f4c into knative:master Mar 18, 2019
@n3wscott n3wscott mentioned this pull request Mar 20, 2019
Closed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@grantr grantr grantr approved these changes

@Harwayne Harwayne Awaiting requested review from Harwayne

@mattmoor mattmoor Awaiting requested review from mattmoor

Assignees

@grantr grantr

@vaikas vaikas

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@n3wscott @evankanderson @knative-prow-robot @grantr @vaikas