ROX-19735: PR suggestions #1287

ludydoo · 2023-09-20T08:38:43Z

No description provided.

openshift-ci · 2023-09-20T08:38:50Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: ludydoo
Once this PR has been reviewed and has the lgtm label, please assign porridge 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

ludydoo · 2023-09-20T09:29:48Z

dev/env/manifests/fleet-manager/04-gitops-config.yaml

+        baseURL: https://raw.githubusercontent.com/stackrox/stackrox/{{ .GitRef }}/operator/bundle/manifests/
+        gitRef: 4.1.1


Do we really need a template here? Why not just specify the url directly

Most of the times it will only be necessary to adjust the GitRef instead of the whole URL. I've used a template in this case because %s seemed not obvious enough if a new engineer not familiar with the config looks at the file.
Example: https://raw.githubusercontent.com/stackrox/stackrox/%s/operator/bundle/manifests/

ludydoo · 2023-09-20T09:30:04Z

dev/env/manifests/fleet-manager/04-gitops-config.yaml

+        gitRef: 4.1.1
+      operators:
+        - gitRef: 4.1.1
+          image: "quay.io/rhacs-eng/stackrox-operator:4.1.1"


I would favor adding the centralLabelSelector here.

Otherwise the engineers will need to know that the Central label (present in patches below) rhacs.redhat.com/version-selector corresponds to the gitRef that is specified in the operators.

What would you prefer, centralVersionSelector or naming the label?

operators: - gitRef: 4.1.1 centralVersionSelector: 4.1.1 image: "quay.io/rhacs-eng/stackrox-operator:4.1.1"

vs naming the label explicitly as a key, it makes it very explicit how the correspond to each other:

operators: - gitRef: 4.1.1 rhacs.redhat.com/version-selector: 4.1.1 image: "quay.io/rhacs-eng/stackrox-operator:4.1.1"

Perhaps the simplest would be to align with the helm values structure?

Seems like the current helm template uses labelSelector as a key

What is the simplest for the user? I think this is the most idiomatic rhacs.redhat.com/version-selector: 4.1.1 value.

ludydoo · 2023-09-20T09:32:44Z

fleetshard/pkg/central/operator/config.go

@@ -17,25 +16,12 @@ func parseConfig(content []byte) (OperatorConfigs, error) {
 	return out, nil
 }

-// GetConfig returns the rhacs operator configurations


ludydoo · 2023-09-20T09:33:01Z

fleetshard/pkg/central/operator/config.go

 		return field.ErrorList{
 			field.Forbidden(path, fmt.Sprintf("could not render operator helm charts, got invalid configuration: %s", err.Error())),
 		}
-	} else if len(manifests) == 0 {


no operators in list is .. undesirable but not "invalid"

It's both, I've intended to catch templating errors. When an empty config is provided it is valid, when config contains operators it is invalid.

} else if len(configs.OperatorConfigs) > 0&& len(manifests) == 0 {

Done and added tests

ludydoo · 2023-09-20T09:33:24Z

fleetshard/pkg/central/reconciler/reconciler.go

-	return r.applyLabelSelector(remoteCentral, central)
-}
-
-func (r *CentralReconciler) applyLabelSelector(remoteCentral *private.ManagedCentral, central *v1alpha1.Central) error {


label is applied using the gitops configMap

How is the default case handled? Does the default Central add the label to all instances outside of a group?

Currently not, but we could either

Specify a path with instanceIds: ["*"] where the label selector is set

Change the default central template.

Other?

SimonBaeumer

Thanks for providing these fixes @ludydoo!

SimonBaeumer · 2023-09-21T08:03:05Z

dev/env/manifests/fleet-manager/04-gitops-config.yaml

+        baseURL: https://raw.githubusercontent.com/stackrox/stackrox/{{ .GitRef }}/operator/bundle/manifests/
+        gitRef: 4.1.1


Most of the times it will only be necessary to adjust the GitRef instead of the whole URL. I've used a template in this case because %s seemed not obvious enough if a new engineer not familiar with the config looks at the file.
Example: https://raw.githubusercontent.com/stackrox/stackrox/%s/operator/bundle/manifests/

SimonBaeumer · 2023-09-21T08:05:48Z

dev/env/manifests/fleet-manager/04-gitops-config.yaml

+        gitRef: 4.1.1
+      operators:
+        - gitRef: 4.1.1
+          image: "quay.io/rhacs-eng/stackrox-operator:4.1.1"


What would you prefer, centralVersionSelector or naming the label?

operators: - gitRef: 4.1.1 centralVersionSelector: 4.1.1 image: "quay.io/rhacs-eng/stackrox-operator:4.1.1"

vs naming the label explicitly as a key, it makes it very explicit how the correspond to each other:

operators: - gitRef: 4.1.1 rhacs.redhat.com/version-selector: 4.1.1 image: "quay.io/rhacs-eng/stackrox-operator:4.1.1"

SimonBaeumer · 2023-09-21T08:08:29Z

fleetshard/pkg/central/operator/config.go

 		return field.ErrorList{
 			field.Forbidden(path, fmt.Sprintf("could not render operator helm charts, got invalid configuration: %s", err.Error())),
 		}
-	} else if len(manifests) == 0 {


It's both, I've intended to catch templating errors. When an empty config is provided it is valid, when config contains operators it is invalid.

} else if len(configs.OperatorConfigs) > 0&& len(manifests) == 0 {

SimonBaeumer · 2023-09-21T08:09:45Z

fleetshard/pkg/central/reconciler/reconciler.go

-	return r.applyLabelSelector(remoteCentral, central)
-}
-
-func (r *CentralReconciler) applyLabelSelector(remoteCentral *private.ManagedCentral, central *v1alpha1.Central) error {


How is the default case handled? Does the default Central add the label to all instances outside of a group?

ROX-19735 PR suggestions

61ba758

ludydoo temporarily deployed to development September 20, 2023 08:38 — with GitHub Actions Inactive

ROX-19735 fix

d23da4d

ludydoo had a problem deploying to development September 20, 2023 08:44 — with GitHub Actions Failure

ludydoo temporarily deployed to development September 20, 2023 08:45 — with GitHub Actions Inactive

ROX-19735 fix

5de9416

ludydoo temporarily deployed to development September 20, 2023 09:08 — with GitHub Actions Inactive

ROX-19735 fix

8297749

ludydoo temporarily deployed to development September 20, 2023 09:24 — with GitHub Actions Inactive

ROX-19735 fix

2e33647

ludydoo temporarily deployed to development September 20, 2023 09:25 — with GitHub Actions Inactive

ROX-19735 fix

1bc514e

ludydoo temporarily deployed to development September 20, 2023 09:27 — with GitHub Actions Inactive

ROX-19735 fix

27ff25d

ludydoo temporarily deployed to development September 20, 2023 09:28 — with GitHub Actions Inactive

ROX-19735 fix

af99ffa

ludydoo temporarily deployed to development September 20, 2023 09:29 — with GitHub Actions Inactive

ludydoo commented Sep 20, 2023

View reviewed changes

ludydoo temporarily deployed to development September 20, 2023 09:30 — with GitHub Actions Inactive

ROX-19735 fix

edd0977

ludydoo temporarily deployed to development September 20, 2023 09:31 — with GitHub Actions Inactive

ludydoo commented Sep 20, 2023

View reviewed changes

ludydoo requested review from SimonBaeumer and kurlov September 21, 2023 07:32

SimonBaeumer reviewed Sep 21, 2023

View reviewed changes

Add test cases

5196468

SimonBaeumer temporarily deployed to development September 21, 2023 08:23 — with GitHub Actions Inactive

SimonBaeumer merged commit 437ce5b into sb/rox-19735-add-operator-to-gitops Sep 21, 2023

SimonBaeumer deleted the lc/rox-19735-suggestions branch September 21, 2023 08:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ROX-19735: PR suggestions #1287

ROX-19735: PR suggestions #1287

ludydoo commented Sep 20, 2023

openshift-ci bot commented Sep 20, 2023

ludydoo Sep 20, 2023

SimonBaeumer Sep 21, 2023

ludydoo Sep 20, 2023 •

edited

Loading

SimonBaeumer Sep 21, 2023 •

edited

Loading

ludydoo Sep 21, 2023

ludydoo Sep 21, 2023

SimonBaeumer Sep 21, 2023

ludydoo Sep 20, 2023

ludydoo Sep 20, 2023

SimonBaeumer Sep 21, 2023

SimonBaeumer Sep 21, 2023

ludydoo Sep 20, 2023

SimonBaeumer Sep 21, 2023

ludydoo Sep 21, 2023

SimonBaeumer left a comment

SimonBaeumer Sep 21, 2023

SimonBaeumer Sep 21, 2023 •

edited

Loading

SimonBaeumer Sep 21, 2023

SimonBaeumer Sep 21, 2023

		baseURL: https://raw.githubusercontent.com/stackrox/stackrox/{{ .GitRef }}/operator/bundle/manifests/
		gitRef: 4.1.1

ROX-19735: PR suggestions #1287

ROX-19735: PR suggestions #1287

Conversation

ludydoo commented Sep 20, 2023

openshift-ci bot commented Sep 20, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ludydoo Sep 20, 2023 • edited Loading

Choose a reason for hiding this comment

SimonBaeumer Sep 21, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SimonBaeumer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SimonBaeumer Sep 21, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ludydoo Sep 20, 2023 •

edited

Loading

SimonBaeumer Sep 21, 2023 •

edited

Loading

SimonBaeumer Sep 21, 2023 •

edited

Loading