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

🐛 Give permission claim controllers their own name #2190

Merged
merged 1 commit into from
Oct 19, 2022
Merged

🐛 Give permission claim controllers their own name #2190

merged 1 commit into from
Oct 19, 2022

Conversation

nrb
Copy link
Contributor

@nrb nrb commented Oct 14, 2022

Summary

Give each of the controllers launched as part of apibindings startup their own name for logging

Related issue(s)

Fixes #1933

@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 14, 2022
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Oct 14, 2022

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

nrb
nrb commented Oct 14, 2022
View reviewed changes
pkg/server/controllers.go Outdated Show resolved Hide resolved
@nrb nrb requested a review from ncdc October 14, 2022 16:01
ncdc
ncdc reviewed Oct 14, 2022
View reviewed changes
pkg/server/controllers.go Outdated Show resolved Hide resolved
ncdc
ncdc suggested changes Oct 14, 2022
View reviewed changes
Copy link
Member

@ncdc ncdc left a comment

Choose a reason for hiding this comment

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

While you're in here, the other controllers in here each all need their own unique name (e.g. deletion)

stevekuznetsov
stevekuznetsov reviewed Oct 17, 2022
View reviewed changes
Copy link
Contributor

@stevekuznetsov stevekuznetsov left a comment

Choose a reason for hiding this comment

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

Can we create individual clients for them as well with unique user agents?

pkg/server/controllers.go Outdated
return err
}

controllerName = "permission-claim-label-resource-controller"
Copy link
Contributor

Choose a reason for hiding this comment

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

Do these controllers export their .Name as a package constant? If not, could they?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, they do. I'll switch things over and make sure they're all pointing at the package vars and add those if they're not present.

ncdc
ncdc reviewed Oct 17, 2022
View reviewed changes
pkg/server/controllers.go Outdated Show resolved Hide resolved
@nrb nrb marked this pull request as ready for review October 17, 2022 19:32
ncdc
ncdc reviewed Oct 17, 2022
View reviewed changes
pkg/server/controllers.go Outdated Show resolved Hide resolved
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 17, 2022
ncdc
ncdc reviewed Oct 17, 2022
View reviewed changes
pkg/server/controllers.go Outdated Show resolved Hide resolved
ncdc
ncdc reviewed Oct 17, 2022
View reviewed changes
pkg/server/controllers.go Outdated Show resolved Hide resolved
ncdc
ncdc reviewed Oct 17, 2022
View reviewed changes
pkg/server/controllers.go Outdated Show resolved Hide resolved
ncdc
ncdc reviewed Oct 17, 2022
View reviewed changes
pkg/server/controllers.go Outdated Show resolved Hide resolved
ncdc
ncdc reviewed Oct 17, 2022
View reviewed changes
pkg/server/controllers.go Outdated Show resolved Hide resolved
@nrb
Give controllers their own names and User-Agent
78faff3 
Signed-off-by: Nolan Brubaker <nolan@nbrubaker.com>
@nrb
Copy link
Contributor Author

nrb commented Oct 17, 2022

e2e-shared-server failed on flake, kcp-dev/contrib-tmc#89

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

openshift-ci bot commented Oct 19, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ncdc

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

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 19, 2022
@nrb
Copy link
Contributor Author

nrb commented Oct 19, 2022

Flake kcp-dev/contrib-tmc#89
/retest

@openshift-merge-robot openshift-merge-robot merged commit 4333fa6 into kcp-dev:main Oct 19, 2022
@kcp-ci-bot kcp-ci-bot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Nov 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ncdc ncdc ncdc approved these changes

@stevekuznetsov stevekuznetsov Awaiting requested review from stevekuznetsov

Assignees

@ncdc ncdc

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/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

bug: multiple controllers share the "kcp-apibinding-controller" user-agent
5 participants
@nrb @ncdc @stevekuznetsov @openshift-merge-robot @kcp-ci-bot