test: remove part-of label expected

[codingben]

What this PR does / why we need it:

Remove the test of expected part-of label from
service tests because in upstream CI we don't
set that label and it's empty.

However, in downstream it's deployed by HCO and
the part-of label is set.

Jira Urls:

1. https://issues.redhat.com/browse/CNV-32226
2. https://issues.redhat.com/browse/CNV-32128

Release note:

None

[openshift-ci]

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

[kubevirt-bot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign ksimon1 for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

• OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[codingben]

/cc @0xFelix

[jcanocan]

IIUC, the label AppKubernetesPartOfLabel is only added on d/s builds, right? If this is the case, tests u/s will always fail.

[codingben]

IIUC, the label AppKubernetesPartOfLabel is only added on d/s builds, right? If this is the case, tests u/s will always fail.

You're right, I think then we can remove that test, it's not really needed.

[sonarcloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
No Duplication information

[codingben]

I removed that test of part-of label. In upstream CI we don't deploy SSP by HCO yet, but in downstream QE CI it's deployed by HCO so the label is set there, but we can't test the value here in upstream.

The fact that we have part-of label set here:

ssp-operator/tests/tests_suite_test.go

Line 114 in 96302e1

common.AppKubernetesPartOfLabel: "hyperconverged-cluster",

Is wrong IMO, and should be removed. We shouldn't set the part-of label since we don't deploy the SSP by HCO here.

[codingben]

/retest

[0xFelix]

It is a feature of SSP to propagate the label from the SSP CR to resources created by SSP. So I think we should absolutely test this feature here.

[codingben]

It is a feature of SSP to propagate the label from the SSP CR to resources created by SSP. So I think we should absolutely test this feature here.

We have 2 controllers that responsible to reconcile different resources:

1. ssp_controller.go - Responsible to reconcile resources that are affected by SSP CR, you can see the whole function of it here [1]. It populates the operand's resources with the same labels as in SSP CR.
2. services_controller.go - It seems like it's not part of the SSP CR and it's not affected by it, according to [2] what I see it only deploys the service object, which has its own labels defined. If there is part-of label defined in the SSP deployment, then it's set the part-of label value (we have it in downstream CSV, the part-of label is set).

I think we shouldn't set the service object with labels that are defined in SSP CR, it's a different controller.

[1] https://github.com/kubevirt/ssp-operator/blob/main/controllers/ssp_controller.go#L132-L239
[2] https://github.com/kubevirt/ssp-operator/blob/main/controllers/services_controller.go#L145-L172

[codingben]

Also @akrejcir mentioned it:

But the ssp-operator-metrics service is created when the operator starts, and is not affected by the SSP CR.

in #608 (comment).

[0xFelix]

Yes, you are right. But I don't see how the part-of label is set in the services_controller.go? In your [2] there is no reference to the part-of label.

I was under the impression that everything deployed by SSP needs to have a part-of label (if specified), but it looks like resources deployed by services_controller.go don't?

[0xFelix]

Ok, when looking at the correct branch I can see the code taking the part-of label. Your [2] points to the deprecated master branch, but in the main branch the code can be found:

ssp-operator/controllers/services_controller.go

Line 156 in 96302e1

appKubernetesPartOfValue := r.deployment.GetLabels()[common.AppKubernetesPartOfLabel]

AFAIU there is no way to modify the deployment easily, so we can't test it US?

[codingben]

Ah sorry about that, I updated the links with main branch.

AFAIU there is no way to modify the deployment easily, so we can't test it US?

I don't see how we can test it in upstream CI. I think in downstream CI we're okay, also the fact that only in downstream there is a requirement for those 4 required labels by HCO QE.

Maybe @akrejcir, do you think we can populate the SSP deployment in upstream CI, when we know that services_controller.go is not affected by SSP CR?

[codingben]

Hi @0xFelix @jcanocan, do you think this PR is needed? If not, can we close it?

[jcanocan]

Hi @0xFelix @jcanocan, do you think this PR is needed? If not, can we close it?

This test is still quarantined in d/s. Could you confirm if without this change the test will pass? If not, IMO is still required, or another way of fixing it.

[0xFelix]

Can you set a static value for common.AppKubernetesPartOfLabel in the Kustomize files and then modify the test to expect that value?