Skip to content

Conversation

@nan-yu
Copy link
Contributor

@nan-yu nan-yu commented Jan 31, 2020

No description provided.

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jan 31, 2020
@nan-yu nan-yu requested a review from mortent January 31, 2020 22:46
@nan-yu
Copy link
Contributor Author

nan-yu commented Jan 31, 2020

/assign @barney-s

@nan-yu
Copy link
Contributor Author

nan-yu commented Feb 3, 2020

@barney-s comments resolved. I did git commit --amend to update the commit. Let me know if you want me to push new commits for later update. Thanks!

return ctrl.Result{}, nil
}

newApplicationStatus.ObservedGeneration = app.Generation
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 do this before we check whether there are any changes to the status and possibly return early? Seems like the application has been observed at the given version even if there are no changes to the status.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we move this before we check whether there are changes to the status, the observedGeneration field will keep increasing. I'll take a deep look into why it is happening. Before we figure it out, I suggest we keep it as is to make sure we get correct observedGeneration field.

Copy link
Contributor

@barney-s barney-s left a comment

Choose a reason for hiding this comment

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

Thanks for rebasing. Minor comments.
Also would it be possible to add an e2e test that uses the docs example recently added.

@barney-s
Copy link
Contributor

barney-s commented Feb 5, 2020

/approve
/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 5, 2020
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: barney-s, nan-yu

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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 5, 2020
@k8s-ci-robot k8s-ci-robot merged commit dbd770a into kubernetes-sigs:master Feb 5, 2020
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. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants