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

configure the operator ServiceAccount #111

Merged
merged 7 commits into from
Oct 12, 2018

Conversation

gregwebs
Copy link
Contributor

@gregwebs gregwebs commented Oct 9, 2018

This allows the serviceAccount to be configured from values.yaml. I am not sure if this should be directly connected to rbac.create or not.

@gregwebs gregwebs requested a review from tennix October 9, 2018 23:16
@tennix
Copy link
Member

tennix commented Oct 10, 2018

When RBAC is enabled, we should create a ServiceAccount for controller-manager. I don't think it's necessary to allow users to specify the ServiceAccount for controller-manager, since we've already created the ServiceAccount named tidb-controller-manager when installing tidb-operator. https://github.com/pingcap/tidb-operator/blob/master/charts/tidb-operator/templates/controller-manager-rbac.yaml#L111

@gregwebs
Copy link
Contributor Author

I added a commit that also makes the ServiceAccount creation optional (even if rbac is true).

tennix
tennix previously approved these changes Oct 12, 2018
Copy link
Member

@tennix tennix left a comment

Choose a reason for hiding this comment

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

LGTM

weekface
weekface previously approved these changes Oct 12, 2018
@weekface
Copy link
Contributor

/run-e2e-tests

1 similar comment
@gregwebs
Copy link
Contributor Author

/run-e2e-tests

@xiaojingchen
Copy link
Contributor

xiaojingchen commented Oct 12, 2018

@gregwebs we need to change images/tidb-operator-values.yaml also when the values.yaml changed, otherwise it can not pass e2e test , because ci use the file to replace the charts/tidb-operator/values.yaml.

@gregwebs gregwebs dismissed stale reviews from weekface and tennix via 3317e9f October 12, 2018 05:41
@tennix
Copy link
Member

tennix commented Oct 12, 2018

/run-e2e-tests

---
kind: ServiceAccount
apiVersion: v1
metadata:
name: tidb-controller-manager
name: {{ .Values.controllerManager.ServiceAccount }}
Copy link
Member

@tennix tennix Oct 12, 2018

Choose a reason for hiding this comment

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

It's serviceAccount not ServiceAccount. User-defined variables are lower case.

@tennix
Copy link
Member

tennix commented Oct 12, 2018

/run-e2e-tests

1 similar comment
@gregwebs
Copy link
Contributor Author

/run-e2e-tests

Copy link
Member

@tennix tennix left a comment

Choose a reason for hiding this comment

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

LGTM

@tennix tennix merged commit 29fd1d1 into pingcap:master Oct 12, 2018
queenliuxx pushed a commit to queenliuxx/tidb-operator that referenced this pull request Dec 19, 2018
* configure the operator ServiceAccount

* make SeriveAccount resource creation optional

* e2e: create the serviceAccount

* fix lower-case serviceAccount
yahonda pushed a commit that referenced this pull request Dec 27, 2021
* Update scale-a-tidb-cluster.md

* Apply suggestions from code review

Co-Authored-By: TomShawn <41534398+TomShawn@users.noreply.github.com>

Co-authored-by: TomShawn <41534398+TomShawn@users.noreply.github.com>
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.

4 participants