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

Add namespace to Prometheus Role #767

Merged
merged 1 commit into from Feb 26, 2021

Conversation

matzhaugen
Copy link
Contributor

@matzhaugen matzhaugen commented Feb 26, 2021

Description

The helm template command does not output a Prometheus Role with a namespace, and will cause the prometheus deployment to crash. This PR adds a namespace to the helm template.

Motivation and Context

See the corresponding issue

  • I have raised an issue to propose this change (required)

How Has This Been Tested?

This has been tested by following the steps in the issue.
Namely,

  1. ./contrib/create_cluster.sh
  2. Create a basic auth secret
  3. Apply the template helm template openfaas ./chart/openfaas --namespace openfaas | kubectl apply -f -

The old setup also works with helm upgrade --install

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I've read the CONTRIBUTION guide
  • I have signed-off my commits with git commit -s
  • I have added tests to cover my changes.
    This is not testable with the current infrastructure. But could be with the setup referred to in this issue
    Open to discussion here
  • All new and existing tests passed.

@derek
Copy link

derek bot commented Feb 26, 2021

Thank you for your contribution. I've just checked and your commit doesn't appear to be signed-off. That's something we need before your Pull Request can be merged. Please see our contributing guide.
Tip: if you only have one commit so far then run: git commit --amend --signoff and then git push --force.

@alexellis
Copy link
Member

A reset would work fine here, or a rebase to squash the commits into one. Here's how I do it:

https://www.youtube.com/watch?v=8j0H6urZ-bU

Where is the resource created when using the first method? Does that match the location after this patch?

@matzhaugen
Copy link
Contributor Author

matzhaugen commented Feb 26, 2021

Before this PR, if you use helm template the role will be installed in the default namespace while helm install will install it to the openfaas namespace. After this PR they will result in the same namespace.

The helm template command does not output a Prometheus Role with a namespace, and will cause the prometheus deployment to crash. This PR adds a namespace to the helm template.

Signed-off-by: Matz Haugen <matz.haugen@cognite.com>
Copy link
Member

@LucasRoesler LucasRoesler left a comment

Choose a reason for hiding this comment

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

Looks good to me

@alexellis alexellis merged commit e65fe3b into openfaas:master Feb 26, 2021
@alexellis
Copy link
Member

Merging now. Thanks for squashing your commits. This will go into the next helm chart release we make.

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

Successfully merging this pull request may close these issues.

None yet

3 participants