-
Notifications
You must be signed in to change notification settings - Fork 130
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
[ISSUE #196] Fix serviceAccountName inconsistency #197
base: master
Are you sure you want to change the base?
Conversation
@@ -27,7 +27,7 @@ spec: | |||
labels: | |||
name: rocketmq-operator | |||
spec: | |||
serviceAccountName: rocketmq-operator | |||
serviceAccountName: {{ template "rocketmq-operator.serviceAccountName" . }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file is copied from that one in deploy directory(see the link below), pls also change the original file.
https://github.com/apache/rocketmq-operator/blob/a8665716c8028741ec106814798e39fe07e6dcaa/Makefile#L78C1-L88
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, I'll go through it once I have time. Please bear with me for a couple of days.
@@ -27,7 +27,7 @@ spec: | |||
labels: | |||
name: rocketmq-operator | |||
spec: | |||
serviceAccountName: rocketmq-operator | |||
serviceAccountName: {{ template "rocketmq-operator.serviceAccountName" . }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Files in deploy
directory are for deploying by kubectl
instead of helm, so templating is not supported.
@drivebyer Pls also take a look, it seems some substitution operations should be added in make manifest
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For now, After we execute make manifest
, the templating in chart
directory disappeared.
I would find some way to do substitution in make manifest.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we can generate chart by helmify, #212
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Finally got some time for this one. So basically, if I understand it right, and correct me if I'm not, that those yaml
files in the deploy
directory don't just serve one purpose which is to be copied to the templates
directory, they are also being used directly by the kubectl
command.
And to fix what I want to fix, something needs to be done in the make manifest
process to properly generate / or recreate / or substitute after copying / or any other ways possible yaml
files with helm templating contents in the templates
directory while at the same time keep yaml
files in the deploy
directory free from helm templating contents.
The thing is, am I allowed to choose whatever means possible to achieve this or should I wait for some discussions to reach a conclusion and then go on with this PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the purpose of the change
Closes #196
Brief changelog
Change the serviceAccountName in the
operator.yaml
to{{ template "rocketmq-operator.serviceAccountName" . }}
, the same as in therole_binding.yaml
andservice_account.yaml
.Verifying this change
This works on a
Kubernetes v1.25.4
cluster with2
nodes, viaHelm version.BuildInfo{Version:"v3.12.0", GitCommit:"c9f554d75773799f72ceef38c51210f1842a1dea", GitTreeState:"clean", GoVersion:"go1.20.3"}
.Please go through this checklist to help us incorporate your contribution quickly and easily.
Notice:
It would be helpful if you could finish the following checklist (the last one is not necessary) before request the community to review your PR
.[ISSUE #123] Fix UnknownException when host config not exist
. Each commit in the pull request should have a meaningful subject line and body.make docker-build
to build docker image for operator, try your changes from Pod inside your Kubernetes cluster, not just locally. Also provide screenshots to show that the RocketMQ cluster is healthy after the changes.make manifests
to make sure the CRD files are updated.