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

Helm: fix namespace issues #2123

Merged
merged 6 commits into from
Jun 21, 2022
Merged

Helm: fix namespace issues #2123

merged 6 commits into from
Jun 21, 2022

Conversation

krajorama
Copy link
Contributor

@krajorama krajorama commented Jun 17, 2022

What this PR does

Add namespace definitions to kubernetes objects so that helm install works correctly even if the namespace does not exist yet.
Add default namespace definition and default namespace selector for ServiceMonitor objects.

Which issue(s) this PR fixes or relates to

Fixes #2045
Based on: #2040

Checklist

  • Tests updated
  • [N/A] Documentation added
  • CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

@krajorama krajorama marked this pull request as draft June 17, 2022 12:37
@krajorama krajorama added the helm label Jun 17, 2022
@krajorama
Copy link
Contributor Author

Should update minio to a newer version as well to get the namespace definitions in minio subchart templates.

Copy link
Contributor

@dimitarvdimitrov dimitarvdimitrov left a comment

Choose a reason for hiding this comment

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

LGTM, two small nits

krajorama and others added 5 commits June 21, 2022 10:07
Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>
Co-authored-by: Rasmus Dencker <rd@firtal.com>
Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>
Co-authored-by: Rasmus Dencker <rd@firtal.com>
Add namespace and namespaceSelector default values.

Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>
Co-authored-by: Rasmus Dencker <rd@firtal.com>
Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>
Does not work in CI as there is no Prom Monitor installed so CRDs are not
defined.

Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>
@krajorama krajorama force-pushed the krajo/20220617-fix-namespace branch from 5d3ea62 to a0b6f88 Compare June 21, 2022 09:39
Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>
@krajorama krajorama marked this pull request as ready for review June 21, 2022 09:55
Copy link
Contributor

@dimitarvdimitrov dimitarvdimitrov left a comment

Choose a reason for hiding this comment

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

tried to look at changes since last time I reviewed. couldn't find any. lgtm

@dimitarvdimitrov
Copy link
Contributor

I testing installing this via helm template and only got the minio resources in the current namespace as opposed to the namespace from the --namespace flag. Are you doing minio in a different PR? are you not doing minio at all? i think it's better if we do that too. As a workaround we can install it in a separate hardocded namespace (mimir-minio?) (see here)

@krajorama krajorama enabled auto-merge (squash) June 21, 2022 11:01
@krajorama krajorama merged commit 9c6e631 into main Jun 21, 2022
@krajorama krajorama deleted the krajo/20220617-fix-namespace branch June 21, 2022 11:13
masonmei pushed a commit to udmire/mimir that referenced this pull request Jul 11, 2022
* Helm: add namespace specification to all objects
* Helm: add some default values for ServiceMonitor objects

Add namespace and namespaceSelector default values.

Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>
Co-authored-by: Rasmus Dencker <rd@firtal.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

helm: Not all resources respect the release namespace
2 participants