Skip to content
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

Parameterize absent pr name #153

Conversation

conallprendergast
Copy link
Contributor

@conallprendergast conallprendergast commented Jan 2, 2024

This PR adds a -rule flag that takes a go template, allowing configuration of the generated absence PrometheusRule API object.
eg

-rule "{{ .metadata.namespace }}-{{ .metadata.name }}-absent-metrics"

Conall Prendergast added 4 commits December 20, 2023 13:53
…r -rule "{{ .Namespace }}-{{ .Name }}-absent-metrics"
…r -rule "{{ .metadata.namespace }}-{{ .metadata.name }}-absent-metrics" 2>&1
Copy link
Contributor

@majewsky majewsky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the late response. I am currently quite busy, but I will try to have one of my teammates look at this soon. As a quick note, can you please run gofmt to clean up the formatting? The easiest way to do this would be through golangci-lint, i.e. make prepare-static-check && golangci-lint run --fix.

Comment on lines +277 to +278
promServer = "default-prometheus"
// return errors.New("no 'prometheus' label found")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like some leftover debugging code

buf := &bytes.Buffer{}
err = t.Execute(buf, m)
if err != nil {
fmt.Println(err.Error())
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use the standard logging methods used elsewhere in the program

@SuperSandro2000
Copy link
Member

Since we wouldn't be using this feature in our environment, can you add a test case which would fail when executed on the code on master, to prevent that the feature regresses in the future?

Also please make sure that static-check is not failing. Right now the existing test suite is failing to compile because one function takes extra arguments.

 ▶ make static-check
>> golangci-lint
test/controller_test.go:1: : # github.com/sapcc/absent-metrics-operator/test [github.com/sapcc/absent-metrics-operator/test.test]
test/controller_test.go:58:59: not enough arguments in call to controllers.AbsencePrometheusRuleName
        have (string)
        want ("github.com/prometheus-operator/prometheus-operator/pkg/apis/monitoring/v1".PrometheusRule, string)
test/controller_test.go:59:59: not enough arguments in call to controllers.AbsencePrometheusRuleName
        have (string)
        want ("github.com/prometheus-operator/prometheus-operator/pkg/apis/monitoring/v1".PrometheusRule, string) (typecheck)
controllers/absence_prometheusrule.go:57:1: paramTypeCombine: func(namespace, name string, promServer string) *monitoringv1.PrometheusRule could be replaced with func(namespace, name, promServer string) *monitoringv1.PrometheusRule (gocritic)
controllers/absence_prometheusrule.go:17: File is not `gofmt`-ed with `-s` (gofmt)
controllers/prometheusrule_controller.go:52: File is not `gofmt`-ed with `-s` (gofmt)
main.go:111: File is not `gofmt`-ed with `-s` (gofmt)
controllers/absence_prometheusrule.go:22: File is not `goimports`-ed with -local github.com/sapcc/absent-metrics-operator (goimports)
controllers/absence_prometheusrule.go:39: unnecessary leading newline (whitespace)
controllers/absence_prometheusrule.go:42:5: ineffectual assignment to err (ineffassign)
controllers/absence_prometheusrule.go:45:2: ineffectual assignment to err (ineffassign)
make: *** [Makefile:70: static-check] Error 1

@talal talal changed the base branch from master to parametrize-promrule-name September 25, 2024 22:07
@talal talal merged commit 2cb900a into sapcc:parametrize-promrule-name Sep 25, 2024
7 of 9 checks passed
talal added a commit that referenced this pull request Sep 26, 2024
talal pushed a commit that referenced this pull request Sep 26, 2024
* Patch metrics operatr to work with a hack

* Working with ./build/absent-metrics-operator -keep-labels service,tier -rule "{{ .Namespace }}-{{ .Name }}-absent-metrics"

* Working with ./build/absent-metrics-operator -keep-labels service,tier -rule "{{ .metadata.namespace }}-{{ .metadata.name }}-absent-metrics" 2>&1

* Clean up

---------

Co-authored-by: Conall Prendergast <[email protected]>
talal pushed a commit that referenced this pull request Sep 26, 2024
* Patch metrics operatr to work with a hack

* Working with ./build/absent-metrics-operator -keep-labels service,tier -rule "{{ .Namespace }}-{{ .Name }}-absent-metrics"

* Working with ./build/absent-metrics-operator -keep-labels service,tier -rule "{{ .metadata.namespace }}-{{ .metadata.name }}-absent-metrics" 2>&1

* Clean up
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants