MON-3468: Add cli args to specify image pullspecs and versions for operants #386

jan--f · 2023-11-09T08:14:57Z

Fixes #250

openshift-ci · 2023-11-09T08:15:01Z

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

pkg/controllers/monitoring/thanos-querier/controller.go

jan--f · 2023-11-09T19:11:39Z

Updated with prometheus/[email protected].

openshift-ci-robot · 2023-11-10T08:49:47Z

@jan--f: This pull request references MON-3468 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.15.0" version, but no target version was set.

In response to this:

Fixes #250

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.

simonpasquier · 2023-11-10T09:42:02Z

cmd/operator/main.go

+	images := kvarg{}
+	flag.Var(&images, "images", "Images to use for containers managed by the observability-operator.")
+	versions := kvarg{}
+	flag.Var(&versions, "versions", "Version of containers managed by the observability-operator.")


Do we need to define the operand versions or should we rely on the hardcoded default versions from github.com/rhobs/obo-prometheus-operator?

I was going to argue for the p-o default versions. In the Thanos Querier controller I already set that explicitly https://github.com/rhobs/observability-operator/pull/386/files#diff-62df723ef5f27edbbfaa7feeb8a0612efaa9ef8d91828004287a17f77f0c48aeR74. In Prometheus objects the version is not set if empty to let p-o do its defaulting.

Defaulting would be latest or does p-o do any kind of logic already?

IMHO it makes more sense for p-o and thanos do their defaults

danielmellado · 2023-11-10T11:55:54Z

test/e2e/framework/assertions.go

 		map[string]float64{
-			`{__name__="controller_runtime_reconcile_errors_total", controller="grafana-operator"}`: 0,


I assume we're removing this as a part of the change as grafana-operator is no longer around in p-o

simonpasquier

FYI I've stumbled across https://pkg.go.dev/k8s.io/[email protected]/cli/flag#MapStringString which can translate key=val CLI args into map[string]string.

jan--f · 2023-11-24T11:09:04Z

FYI I've stumbled across https://pkg.go.dev/k8s.io/[email protected]/cli/flag#MapStringString which can translate key=val CLI args into map[string]string.

Nice, thank you! Added.

Signed-off-by: Jan Fajerski <[email protected]>

This commit also decouples the monitoring-stack controller and thanos controller by implementing a ThanosSidecar configuration struct in monitoring-stack. Signed-off-by: Jan Fajerski <[email protected]>

danielmellado · 2024-01-18T11:17:36Z

hmmmm e2e tests are complaining about mv: cannot stat 'tmp/e2e': No such file or directory let's see

danielmellado · 2024-01-18T12:43:08Z

/test observability-operator-e2e

danielmellado · 2024-01-18T13:11:27Z

/assign @sthaha

And add some default checking and setting. Signed-off-by: Jan Fajerski <[email protected]>

cmd/operator/main.go

pkg/operator/operator.go

Signed-off-by: Jan Fajerski <[email protected]>

simonpasquier · 2024-01-18T15:10:30Z

cmd/operator/main.go

+// error.
+func validateImages(images *k8sflag.MapStringString) (map[string]string, error) {
+	res := defaultImages
+	if !images.Empty() {


(nit) fast path if no custom images are provided

Suggested change

if !images.Empty() {

if images.Empty() {

return res

}

simonpasquier · 2024-01-18T15:10:57Z

cmd/operator/main.go

+		imgs := *images.Map
+		for k, v := range imgs {
+			if _, ok := res[k]; !ok {
+				return nil, fmt.Errorf(fmt.Sprintf("image %v is unknown", k))


Suggested change

return nil, fmt.Errorf(fmt.Sprintf("image %v is unknown", k))

return nil, fmt.Errorf("image %v is unknown", k)

simonpasquier · 2024-01-18T15:13:13Z

cmd/operator/main.go

+func imagesUsed() []string {
+	i := 0
+	imgs := make([]string, len(defaultImages))
+	for k := range defaultImages {
+		imgs[i] = k
+		i++
+	}
+	return imgs
+}


(nit) ensure stable sort

Suggested change

func imagesUsed() []string {

i := 0

imgs := make([]string, len(defaultImages))

for k := range defaultImages {

imgs[i] = k

i++

}

return imgs

}

func imagesUsed() []string {

i := 0

imgs := make([]string, len(defaultImages))

for k := range defaultImages {

imgs[i] = k

i++

}

slices.Sortimgs)

return imgs

}

Signed-off-by: Jan Fajerski <[email protected]>

danielmellado · 2024-01-18T17:14:11Z

/lgtm

danielmellado

lgtm

openshift-ci · 2024-01-18T17:15:39Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: danielmellado, jan--f

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:

~~OWNERS~~ [danielmellado,jan--f]

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

openshift-ci bot added the do-not-merge/work-in-progress label Nov 9, 2023

openshift-merge-robot added the needs-rebase label Nov 9, 2023

openshift-ci bot added the approved label Nov 9, 2023

jan--f force-pushed the image-version-cli-args branch from 5c8669b to f8ec622 Compare November 9, 2023 09:13

openshift-merge-robot removed the needs-rebase label Nov 9, 2023

simonpasquier reviewed Nov 9, 2023

View reviewed changes

pkg/controllers/monitoring/thanos-querier/controller.go Show resolved Hide resolved

jan--f force-pushed the image-version-cli-args branch from f8ec622 to 8bde370 Compare November 9, 2023 19:10

jan--f force-pushed the image-version-cli-args branch from 8bde370 to 61da795 Compare November 9, 2023 19:17

jan--f marked this pull request as ready for review November 9, 2023 19:20

jan--f requested a review from a team as a code owner November 9, 2023 19:20

jan--f requested review from simonpasquier and sthaha and removed request for a team November 9, 2023 19:20

openshift-ci bot removed the do-not-merge/work-in-progress label Nov 9, 2023

openshift-ci bot requested review from lihongyan1 and marioferh November 9, 2023 19:20

jan--f force-pushed the image-version-cli-args branch 2 times, most recently from dccb879 to af2046d Compare November 10, 2023 08:20

jan--f changed the title ~~Add cli args to specify image pullspecs and versions for operants~~ MON-3468: Add cli args to specify image pullspecs and versions for operants Nov 10, 2023

openshift-ci-robot added the jira/valid-reference label Nov 10, 2023

simonpasquier reviewed Nov 10, 2023

View reviewed changes

danielmellado reviewed Nov 10, 2023

View reviewed changes

openshift-merge-robot added the needs-rebase label Nov 14, 2023

simonpasquier reviewed Nov 15, 2023

View reviewed changes

jan--f force-pushed the image-version-cli-args branch from af2046d to a2dee86 Compare November 24, 2023 11:09

openshift-merge-robot removed the needs-rebase label Nov 24, 2023

jan--f force-pushed the image-version-cli-args branch from 3f0baf2 to a4d5ccc Compare January 18, 2024 07:38

jan--f added 4 commits January 18, 2024 10:14

feat: add cli flags for operant images and versions

eccf936

Signed-off-by: Jan Fajerski <[email protected]>

refactor: use k8s cli flag type MapStringString

4796e7f

Signed-off-by: Jan Fajerski <[email protected]>

refactor: pass around component versions in a struct

53c5d86

Signed-off-by: Jan Fajerski <[email protected]>

refactor: --images should a full image ref

f46bc1b

This commit also decouples the monitoring-stack controller and thanos controller by implementing a ThanosSidecar configuration struct in monitoring-stack. Signed-off-by: Jan Fajerski <[email protected]>

jan--f force-pushed the image-version-cli-args branch from a4d5ccc to 8096284 Compare January 18, 2024 09:15

openshift-merge-robot removed the needs-rebase label Jan 18, 2024

jan--f force-pushed the image-version-cli-args branch from 8096284 to b301231 Compare January 18, 2024 09:33

openshift-ci bot assigned sthaha Jan 18, 2024

jan--f force-pushed the image-version-cli-args branch from b301231 to 300696d Compare January 18, 2024 13:50

refactor: set operator options via functional options

c6b064c

And add some default checking and setting. Signed-off-by: Jan Fajerski <[email protected]>

jan--f force-pushed the image-version-cli-args branch from 300696d to c6b064c Compare January 18, 2024 13:59

danielmellado reviewed Jan 18, 2024

View reviewed changes

cmd/operator/main.go Outdated Show resolved Hide resolved

simonpasquier reviewed Jan 18, 2024

View reviewed changes

cmd/operator/main.go Outdated Show resolved Hide resolved

cmd/operator/main.go Outdated Show resolved Hide resolved

pkg/operator/operator.go Outdated Show resolved Hide resolved

danielmellado reviewed Jan 18, 2024

View reviewed changes

pkg/operator/operator.go Outdated Show resolved Hide resolved

fix: adjust managed_fields_in_Prometheus_object to image feature

51d028d

Signed-off-by: Jan Fajerski <[email protected]>

jan--f force-pushed the image-version-cli-args branch from eb4c6c8 to c3ff294 Compare January 18, 2024 14:58

simonpasquier reviewed Jan 18, 2024

View reviewed changes

refactor: address review comments

324db3c

Signed-off-by: Jan Fajerski <[email protected]>

jan--f force-pushed the image-version-cli-args branch from c3ff294 to 324db3c Compare January 18, 2024 16:06

chore: use go 1.21 in Dockerfiles

b925b36

Signed-off-by: Jan Fajerski <[email protected]>

openshift-ci bot assigned danielmellado Jan 18, 2024

openshift-ci bot added the lgtm label Jan 18, 2024

danielmellado approved these changes Jan 18, 2024

View reviewed changes

openshift-merge-bot bot merged commit c0f54ae into rhobs:main Jan 18, 2024
11 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MON-3468: Add cli args to specify image pullspecs and versions for operants #386

MON-3468: Add cli args to specify image pullspecs and versions for operants #386

jan--f commented Nov 9, 2023 •

edited

Loading

openshift-ci bot commented Nov 9, 2023

jan--f commented Nov 9, 2023

openshift-ci-robot commented Nov 10, 2023 •

edited by openshift-ci bot

Loading

simonpasquier Nov 10, 2023

jan--f Nov 10, 2023

danielmellado Nov 10, 2023

vkmc Jan 9, 2024

danielmellado Nov 10, 2023

simonpasquier left a comment

jan--f commented Nov 24, 2023

danielmellado commented Jan 18, 2024

danielmellado commented Jan 18, 2024

danielmellado commented Jan 18, 2024

simonpasquier Jan 18, 2024

simonpasquier Jan 18, 2024

simonpasquier Jan 18, 2024

danielmellado commented Jan 18, 2024

danielmellado left a comment

openshift-ci bot commented Jan 18, 2024

		map[string]float64{
		`{__name__="controller_runtime_reconcile_errors_total", controller="grafana-operator"}`: 0,

-	if !images.Empty() {
+	if images.Empty() {
+		return res
+	}

	return nil, fmt.Errorf(fmt.Sprintf("image %v is unknown", k))
	return nil, fmt.Errorf("image %v is unknown", k)

MON-3468: Add cli args to specify image pullspecs and versions for operants #386

MON-3468: Add cli args to specify image pullspecs and versions for operants #386

Conversation

jan--f commented Nov 9, 2023 • edited Loading

openshift-ci bot commented Nov 9, 2023

jan--f commented Nov 9, 2023

openshift-ci-robot commented Nov 10, 2023 • edited by openshift-ci bot 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

simonpasquier left a comment

Choose a reason for hiding this comment

jan--f commented Nov 24, 2023

danielmellado commented Jan 18, 2024

danielmellado commented Jan 18, 2024

danielmellado commented Jan 18, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danielmellado commented Jan 18, 2024

danielmellado left a comment

Choose a reason for hiding this comment

openshift-ci bot commented Jan 18, 2024

jan--f commented Nov 9, 2023 •

edited

Loading

openshift-ci-robot commented Nov 10, 2023 •

edited by openshift-ci bot

Loading