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

fix(helm): add conditional namespace on watchAnyNamespace true #9995

Merged
merged 1 commit into from
Apr 19, 2024

Conversation

FlxPeters
Copy link
Contributor

When the feature flag is enabled, the ClusterRoleBinding has a namespace which is not valid. In order to fix this a condition for the namespace is added to all relevant templates.

Type of change

Select the type of your PR

  • Bugfix

Description

Please describe your pull request

Checklist

Please go through this checklist and make sure all applicable tasks have been done

  • Write tests
  • Make sure all tests pass
  • Update documentation
  • Check RBAC rights for Kubernetes / OpenShift roles
  • Try your changes from Pod inside your Kubernetes and OpenShift cluster, not just locally
  • Reference relevant issue(s) and close them after merging
  • Update CHANGELOG.md
  • Supply screenshots for visual changes, such as Grafana dashboards

@FlxPeters FlxPeters changed the title fix: add conditional namespace on watchAnyNamespace true fix(helm): add conditional namespace on watchAnyNamespace true Apr 18, 2024
Copy link
Member

@scholzj scholzj 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 the PR. I guess the debug.yaml should not be there? Also, you should change only the packaging folder. The /helm-charts folder is updated only as part of the release.

@FlxPeters
Copy link
Contributor Author

Thanks for the PR. I guess the debug.yaml should not be there? Also, you should change only the packaging folder. The /helm-charts folder is updated only as part of the release.

Yep, just missed that one when commiting. Now all should be fine.

@scholzj
Copy link
Member

scholzj commented Apr 18, 2024

Thanks for the PR. I guess the debug.yaml should not be there? Also, you should change only the packaging folder. The /helm-charts folder is updated only as part of the release.

Yep, just missed that one when commiting. Now all should be fine.

Sorry if I confused you ... but it looks like you did it the other way around -> /packaging/hlem-charts is where you should do the change. But /helm-charts has to remain unchanged.

@FlxPeters
Copy link
Contributor Author

I was also confused by my own commit. I think it should be fine now?

@scholzj
Copy link
Member

scholzj commented Apr 18, 2024

I was also confused by my own commit. I think it should be fine now?

No, you have the changes in the /helm-charts directory. They have to be packaging/helm-charts only.

When the feature flag is enabled, the ClusterRoleBinding has a namespace which is not valid. In order to fix this a condition for the namespace is added to all relevant templates.

Signed-off-by: Felix Peters <felix.peters@breuninger.de>
@FlxPeters
Copy link
Contributor Author

Oh, I feel some kind of dump right now... sorry for the chaos.
Now it should be finally fixed. My bad.

@scholzj scholzj added this to the 0.41.0 milestone Apr 18, 2024
Copy link
Member

@scholzj scholzj left a comment

Choose a reason for hiding this comment

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

That looks good now. Thanks.

@scholzj
Copy link
Member

scholzj commented Apr 18, 2024

/azp run helm-acceptance

@scholzj scholzj requested a review from ppatierno April 18, 2024 21:34
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Copy link
Member

@ppatierno ppatierno left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for the PR!

@scholzj scholzj merged commit 6c3cec5 into strimzi:main Apr 19, 2024
15 checks passed
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.

None yet

3 participants