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

Release namespace for Prometheus Operator resources #967

Merged
merged 6 commits into from
Nov 23, 2023

Conversation

bennesp
Copy link
Contributor

@bennesp bennesp commented Nov 17, 2023

What does this PR do?

Set namespace for ServiceMonitor and PrometheusRule resources to the release namespace if no namespace is set into the values file, like all others resources

Motivation

Closes #948 which seems to be stale

More

  • Yes, I updated the tests accordingly
  • Yes, I ran make test and all the tests passed

nlamirault and others added 3 commits October 26, 2023 15:14
Signed-off-by: Nicolas Lamirault <nicolas.lamirault@gmail.com>
Signed-off-by: bennesp <bennesp@users.noreply.github.com>
Signed-off-by: bennesp <bennesp@users.noreply.github.com>
@mloiseleur mloiseleur added the kind/enhancement New feature or request label Nov 18, 2023
Copy link
Contributor

@mloiseleur mloiseleur left a comment

Choose a reason for hiding this comment

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

Thanks for this PR !

In order keep good readability, we try avoid having

{{- if x }}
field: value
{{- else }}
field: othervalue
{{- end }}

Would you please use a template or a ternary instead ?

You should also start with the test, on both objects. The tests should fail without your changes on the template.

@bennesp
Copy link
Contributor Author

bennesp commented Nov 18, 2023

Can you clarify this sentence please?

> You should also start with the test, on both objects. The tests should fail without your changes on the template.

I am asking because tests are already in place, and they fails without these changes already, so I am a bit confused

Apologies, I got now that you were referring to the other template that the original author modified, will write tests also for that template

Signed-off-by: bennesp <bennesp@users.noreply.github.com>
Signed-off-by: bennesp <bennesp@users.noreply.github.com>
Copy link
Contributor

@mloiseleur mloiseleur left a comment

Choose a reason for hiding this comment

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

LGTM

@mloiseleur
Copy link
Contributor

Apologies, I got now that you were referring to the other template that the original author modified, will write tests also for that template

No problem. Thanks for rebasing and updating the PR. It looks better now.

Copy link
Contributor

@darkweaver87 darkweaver87 left a comment

Choose a reason for hiding this comment

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

LGTM

@traefiker traefiker merged commit 9c83a2a into traefik:master Nov 23, 2023
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants