Skip to content

Update status.ObservedGeneration for sources resources when sources r…#1897

Merged
knative-prow-robot merged 2 commits into
knative:masterfrom
capri-xiyue:issue_1893
Sep 16, 2019
Merged

Update status.ObservedGeneration for sources resources when sources r…#1897
knative-prow-robot merged 2 commits into
knative:masterfrom
capri-xiyue:issue_1893

Conversation

@capri-xiyue
Copy link
Copy Markdown
Contributor

@capri-xiyue capri-xiyue commented Sep 16, 2019

Fixes #1893

Proposed Changes

  • Update status.ObservedGeneration for sources resources when sources resources get reconciled successfully
  • Added unit tests

Release Note

None

@capri-xiyue
Update status.ObservedGeneration for sources resources when sources r…
7d8a2f2 
…esources get reconciled successfully, added unit tests
@googlebot googlebot added the cla: yes Indicates the PR's author has signed the CLA. label Sep 16, 2019
@knative-prow-robot knative-prow-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Sep 16, 2019
@capri-xiyue
Copy link
Copy Markdown
Contributor Author

capri-xiyue commented Sep 16, 2019

/assign @Harwayne

Harwayne
Harwayne reviewed Sep 16, 2019
View reviewed changes
Comment thread pkg/reconciler/apiserversource/apiserversource.go Outdated
return err
}
source.Status.MarkEventTypes()
source.Status.ObservedGeneration = source.Generation
Copy link
Copy Markdown
Contributor

@Harwayne Harwayne Sep 16, 2019

Choose a reason for hiding this comment

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

Should this be at the beginning of the reconcile loop? My reasoning being that whatever status this reconcile loop returns, it is associated with source.Generation, not whatever the old value of source.Status.ObservedGeneration was.

Copy link
Copy Markdown
Contributor Author

@capri-xiyue capri-xiyue Sep 16, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Discussed offline. At first, I followed the logic when knative serving update the Status.ObservedGeneration for resources. Talked with devs of Knative serving, they said they will work on update the Status.ObservedGeneration for resources at the beginning of the reconcile loop instead of at the end of the reconcile loop because that's the definition of ObservedGeneration( ObservedGeneration is the 'Generation' of the Service that was last processed by the controller). ObservedGeneration is the 'Generation' of the Service that was last processed by the controller no matter the reconcile is successful or not. Will change the code to update at the beginning of the reconcile loop.

@knative-metrics-robot
Copy link
Copy Markdown

knative-metrics-robot commented Sep 16, 2019

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

File Old Coverage New Coverage Delta
pkg/reconciler/apiserversource/apiserversource.go 77.5% 77.6% 0.1
pkg/reconciler/containersource/containersource.go 80.2% 80.3% 0.2
pkg/reconciler/cronjobsource/cronjobsource.go 69.2% 68.7% -0.5

Harwayne
Harwayne reviewed Sep 16, 2019
View reviewed changes
Copy link
Copy Markdown
Contributor

@Harwayne Harwayne left a comment

Choose a reason for hiding this comment

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

/approve
/lgtm

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

knative-prow-robot commented Sep 16, 2019

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: capri-xiyue, Harwayne

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 Sep 16, 2019
@knative-prow-robot knative-prow-robot merged commit 0124584 into knative:master Sep 16, 2019
matzew added a commit to matzew/eventing that referenced this pull request Oct 26, 2022
@matzew
[release-v1.5] Setting to 1.21 (matching ocp 4.8) (knative#1897)
62f5f61 
* Setting to 1.21 (matching ocp 4.8)

Signed-off-by: Matthias Wessendorf <mwessend@redhat.com>

* updating patch as source of this change as well

Signed-off-by: Matthias Wessendorf <mwessend@redhat.com>

Signed-off-by: Matthias Wessendorf <mwessend@redhat.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@grantr grantr Awaiting requested review from grantr

@vaikas vaikas Awaiting requested review from vaikas

1 more reviewer

@Harwayne Harwayne Harwayne left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@Harwayne Harwayne

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/L Denotes a PR that changes 100-499 lines, ignoring generated files.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Update status.ObservedGeneration for sources resources when sources resources get reconciled successfully

5 participants

@capri-xiyue @knative-metrics-robot @knative-prow-robot @Harwayne @googlebot