Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

🐛 syncer/status: make sure the resource is owned by this syncer before updating the status #2049

Merged
merged 2 commits into from
Sep 28, 2022
Merged

🐛 syncer/status: make sure the resource is owned by this syncer before updating the status #2049

merged 2 commits into from
Sep 28, 2022

Conversation

jmprusi
Copy link
Member

@jmprusi jmprusi commented Sep 22, 2022

Summary

The syncer Status controller was missing a validation check, so it could update the wrong resource status field in certain cases.

Related issue(s)

Fixes #2048

@openshift-ci openshift-ci bot requested review from ncdc and sttts September 22, 2022 12:47
@jmprusi
Copy link
Member Author

jmprusi commented Sep 22, 2022

/hold fixing tests

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 22, 2022
@jmprusi
Copy link
Member Author

jmprusi commented Sep 22, 2022

/unhold

@openshift-ci openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 22, 2022
@sttts
Copy link
Member

sttts commented Sep 23, 2022

/lgtm
/approve

@openshift-ci openshift-ci bot added lgtm Indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Sep 23, 2022
@jmprusi
Copy link
Member Author

jmprusi commented Sep 23, 2022

@sttts, can you force the merge without considering the lint test? It's complaining about the structured logging, which this PR has nothing to do with, and there's another PR where the conversion to structured logging is handled. Thanks!

@ncdc
Copy link
Member

ncdc commented Sep 23, 2022

/hold

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 23, 2022
@ncdc
Copy link
Member

ncdc commented Sep 23, 2022

@jmprusi please follow the instructions re lint:

[INFO] If you need to update the saved list, run `make update-contextual-logging` and commit `hack/logcheck.out`'

Thanks!

@ncdc
Copy link
Member

ncdc commented Sep 23, 2022

If we force merge without fixing the lint errors, main + every future PR will have broken lint

@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Sep 23, 2022
@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 23, 2022
@jmprusi jmprusi force-pushed the jmprusi/fix-incorrect-status-update branch from e42b5de to 40e98a5 Compare September 27, 2022 23:23
@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 27, 2022
@jmprusi
Copy link
Member Author

jmprusi commented Sep 28, 2022

@ncdc fixed! thanks!

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Sep 28, 2022
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Sep 28, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: davidfestal, sttts

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

@davidfestal
Copy link
Member

/unhold

@openshift-ci openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 28, 2022
@davidfestal
Copy link
Member

@ncdc Unholded the PR since lint errors have been fixed.

@openshift-merge-robot openshift-merge-robot merged commit d2e9c98 into kcp-dev:main Sep 28, 2022
@ncdc ncdc mentioned this pull request Oct 5, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@davidfestal davidfestal davidfestal approved these changes

@ncdc ncdc Awaiting requested review from ncdc

@sttts sttts Awaiting requested review from sttts

Assignees

@davidfestal davidfestal

@sttts sttts

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.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

bug: Statuses of OpenShift Routes are synced to wrong upstream instances
5 participants
@jmprusi @sttts @ncdc @davidfestal @openshift-merge-robot