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

Cber/switch for nginx metrics #127

Merged
merged 13 commits into from
Feb 26, 2024
Merged

Conversation

ChristianBergen
Copy link
Contributor

Adds a switch to be able to disable the metrics in the nginx_values.yaml

@schwichti
Copy link
Member

Can you elaborate a bit why we need a switch for the metrics?

@amarin-dspace
Copy link
Contributor

Maybe it's for the best, since we currently depend on simphera-monitoring helm chart to deploy necessary CRDs, in order to have metrics enabled and deployed with nginx helm chart.
There is PR in simphera admin manual explaining the dependency of the two helm charts, but in order to avoid failed deployments (by default), I think this PR makes a lot of sense.

@ChristianBergen
Copy link
Contributor Author

Yes, the case is, that our infrastructure creations fail because of the "metrics enabled" values in the nginx_values.yaml. By default they would still be enabled, but we as SystemTeam do not use this feature currently.

variables.tf Outdated
@@ -257,3 +257,8 @@ variable "cluster_autoscaler_helm_config" {
default = {}
}

variable "enable_nginx_metrics" {
type = bool
description = "Switch to enable the metrics in the nginx values"
Copy link
Contributor

Choose a reason for hiding this comment

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

"Switch to enable nginx metrics"?

variables.tf Outdated
variable "enable_nginx_metrics" {
type = bool
description = "Switch to enable the metrics in the nginx values"
default = true
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe to have it 'false' by default and put a small update to README file?

Copy link
Contributor

Choose a reason for hiding this comment

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

I mean, have the var present, but set its default value from 'true' to 'false'

templates/nginx_values.yaml Outdated Show resolved Hide resolved
templates/nginx_values.yaml Outdated Show resolved Hide resolved
@amarin-dspace
Copy link
Contributor

LGTM, as we agreed in discussion with SimonS, we'll abandon these changes in TF and move nginx metrics' part completely to Simphera monitoring chart.

@ChristianBergen ChristianBergen merged commit 052d8aa into main Feb 26, 2024
@ChristianBergen ChristianBergen deleted the cber/switch_for_nginx_metrics branch February 26, 2024 14:37
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.

3 participants