-
Notifications
You must be signed in to change notification settings - Fork 47
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.
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
fix: replacing of namespace annotation when openshift-cnv namespace is set #683
Conversation
/cherrypick release-v0.18 |
@ksimon1: once the present PR merges, I will cherry-pick it on top of release-v0.18 in a new PR and assign it to you. In response to this:
Instructions 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. |
|
||
key := client.ObjectKeyFromObject(&configMap) | ||
cm := &v1.ConfigMap{} | ||
ExpectWithOffset(1, request.Client.Get(request.Context, key, cm)).ToNot(HaveOccurred()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using ExpectWithOffset()
here does not make sense, because this is the top level of the test case.
|
||
key := client.ObjectKeyFromObject(&roleBinding) | ||
rb := &rbac.RoleBinding{} | ||
ExpectWithOffset(1, request.Client.Get(request.Context, key, rb)).ToNot(HaveOccurred()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The same here.
3c673b7
to
9e737a4
Compare
In the future, we should probably fix this in a better way. But it is ok for now. /lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: akrejcir 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 |
/hold This looks like a crude workaround to me, can't it be fixed properly immediately? |
What other solution do you suggest? |
Why? |
Because we don't know in which different namespace the results will go. According to discussions we want to target to a single user defined namespace to allow regular user to use our pipelines. Thats why ssp operator deploys everything into single namespace when there is a namespace defined in ssp cr. |
9e737a4
to
e80fd45
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few comments, I'm also not seeing any additional test coverage for your changes in Cleanup
?
@ksimon1: The following tests failed, say
Full PR test history. Your PR dashboard. Instructions 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. |
in ssp CR When ssp CR contains kubevirt namespace as a target namespace for pipelines, ssp operator recognized this namespace as user defined and deployed configmaps and roleBindings to a single namespace, instead of two namespaces. That broke our default pipeline flow. This change adds a check if the pipeline namespace in the CR is kubevirt, then it is not used as a user defined namespace and kubevirt.io/tekton-piplines-deploy-namespace annotation is used instead. rename kubevirt.io/deploy-namespace annotation to kubevirt.io/tekton-piplines-deploy-namespace Signed-off-by: Karel Simon <[email protected]>
e80fd45
to
25dd686
Compare
Kudos, SonarCloud Quality Gate passed! 0 Bugs No Coverage information |
This is already covered by test which already exists - https://github.com/kubevirt/ssp-operator/blob/main/internal/operands/tekton-pipelines/reconcile_test.go#L114 |
/lgtm |
/unhold |
Skipping pipelines tests, because they are not working due to https://bugzilla.redhat.com/show_bug.cgi?id=2236223 |
@ksimon1: new pull request created: #687 In response to this:
Instructions 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. |
What this PR does / why we need it:
fix: replacing of namespace annotation when kubevirt namespace is set
When ssp CR contains kubevirt namespace as a target namespace
for pipelines, ssp operator recognized this namespace as user defined
and deployed configmaps and roleBindings to a single namespace, instead
of two namespaces. That broke our default pipeline flow. This
change adds a check if the pipeline namespace in the CR is kubevirt,
then it is not used as a user defined namespace and
kubevirt.io/deploy-namespace annotation is used instead.
Fixes https://bugzilla.redhat.com/show_bug.cgi?id=2237916
Special notes for your reviewer:
Release note: