[WIP] Label syncer labelling #366
Conversation
Signed-off-by: perdasilva <[email protected]>
openshift-ci
bot
added
the
do-not-merge/work-in-progress
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: perdasilva The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
perdasilva
force-pushed
the
label_syncer_labelling
branch
from
9ad97e1 to
6a76a28
Compare
openshift-ci
bot
added
the
approved
perdasilva
force-pushed
the
label_syncer_labelling
branch
from
6a76a28 to
ebe976e
Compare
Signed-off-by: perdasilva <[email protected]>
perdasilva
force-pushed
the
label_syncer_labelling
branch
2 times, most recently
from
91120c2 to
d6b8145
Compare
Signed-off-by: perdasilva <[email protected]> Upstream-repository: perdasilva Upstream-commit: baaafa10f8d9f5c3251bfccb207c20bb84cdbc80 Signed-off-by: perdasilva <[email protected]>
perdasilva
force-pushed
the
label_syncer_labelling
branch
from
d6b8145 to
6679d14
Compare
joelanford
left a comment
joelanford
left a comment
There was a problem hiding this comment.
Looks pretty good to me. Only question I have is whether its possible to hook and extra event handler into the existing CSV informer that we already use in the olm-operator?
If we could do that, we wouldn't need this extra informer.
| // payload, as retrieved by listing the namespaces after a successful installation | ||
| // IMPORTANT: The Namespace openshift-operators must be an exception to this rule | ||
| // since it is used by OCP/OLM users to install their Operator bundle solutions. | ||
| var systemNSSyncExemptions = sets.NewString( |
There was a problem hiding this comment.
I take it this is the exact same list as the label syncer's? Any chance we could get them to put that somewhere we could pull in? If not, how do we keep this up-to-date?
There was a problem hiding this comment.
It is - I just took it from their code for now. But yet, we definitely need to collapse into a single source of truth.
| op.client, | ||
| namespace, | ||
| func(opts *metav1.ListOptions) { | ||
| // opts.LabelSelector = fmt.Sprintf("!%s", v1alpha1.CopiedLabelKey) |
There was a problem hiding this comment.
It was a premature optimization. While it can help reduce the memory footprint, it means a 'not found' error when you try to get the namespace (you might still get csv events from csvs in namespaces that are culled). This felt a bit ambiguous and a potential source of confusion for the next developer. wdyt?
There was a problem hiding this comment.
sorry, I confused myself here. The reason I commented this one out was because I was getting some strange behavior. I wasn't getting events for a non-openshift-* namespace...? I will try again to make sure though.
|
@perdasilva: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
Bumps [k8s.io/api](https://github.com/kubernetes/api) from 0.31.1 to 0.31.2. - [Commits](kubernetes/api@v0.31.1...v0.31.2) --- updated-dependencies: - dependency-name: k8s.io/api dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Upstream-repository: api Upstream-commit: 769c5e44dc3a5795c106d5e599b53d9a60b32891
2 participants
No description provided.