You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by GitBox <gi...@apache.org> on 2021/12/28 15:19:32 UTC
[GitHub] [superset] wiktor2200 opened a new pull request #17880: feat: Custom service account creation and management
wiktor2200 opened a new pull request #17880:
URL: https://github.com/apache/superset/pull/17880
<!---
Please write the PR title following the conventions at https://www.conventionalcommits.org/en/v1.0.0/
Example:
fix(dashboard): load charts correctly
-->
### SUMMARY
<!--- Describe the change below, including rationale and design decisions -->
Possibility to create custom service account in helm chart.
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
<!--- Skip this if not applicable -->
### TESTING INSTRUCTIONS
<!--- Required! What steps can be taken to manually verify the changes? -->
I've tested all possible changes on parameters:
* serviceAccountName
* serviceAccount.create
All conditionals in `_helpers` file works as expected.
### ADDITIONAL INFORMATION
<!--- Check any relevant boxes with "x" -->
<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
- [x] Has associated issue: #17879 https://github.com/apache/superset/issues/17879
- [ ] Required feature flags:
- [ ] Changes UI
- [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] Migration is atomic, supports rollback & is backwards-compatible
- [ ] Confirm DB migration upgrade and downgrade tested
- [ ] Runtime estimates and downtime expectations provided
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] wiktor2200 commented on pull request #17880: feat: Helm - custom service account creation and management
Posted by GitBox <gi...@apache.org>.
wiktor2200 commented on pull request #17880:
URL: https://github.com/apache/superset/pull/17880#issuecomment-1002579881
@przemyslawandruszewski I've added support for custom service account in init-job in this commit: https://github.com/apache/superset/pull/17880/commits/372fed158346767d24305dd185bed448a295107d
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] wiktor2200 commented on a change in pull request #17880: feat: Helm - custom service account creation and management
Posted by GitBox <gi...@apache.org>.
wiktor2200 commented on a change in pull request #17880:
URL: https://github.com/apache/superset/pull/17880#discussion_r776308752
##########
File path: helm/superset/values.yaml
##########
@@ -25,6 +25,11 @@ replicaCount: 1
# Runn containers as root is not recommended in production. Change this to another UID - e.g. 1000 to be more secure
runAsUser: 0
+# Create custom service account for Superset. If create: true and name is not provided, superset.fullname will be used.
+# serviceAccountName: superset
+serviceAccount:
+ create: false
Review comment:
I've thought about it. That was even my first choice, but then I've noticed that, there was `ServiceAccountName` variable added in the past #15340, so I've implemented new feature as enhancement beside the current solution.
I don't want to destroy any configs that can exist somewhere.
@mvoitko and @craig-rueda can you give some more feedback about this?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] przemyslawandruszewski commented on pull request #17880: feat: Helm - custom service account creation and management
Posted by GitBox <gi...@apache.org>.
przemyslawandruszewski commented on pull request #17880:
URL: https://github.com/apache/superset/pull/17880#issuecomment-1004610438
> Unfortunately I can't. :( It has to be clicked by someone with write access to repo. 
Sorry then :) So the request goes to @craig-rueda :)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] przemyslawandruszewski commented on pull request #17880: feat: Helm - custom service account creation and management
Posted by GitBox <gi...@apache.org>.
przemyslawandruszewski commented on pull request #17880:
URL: https://github.com/apache/superset/pull/17880#issuecomment-1002622166
@wiktor2200 I cannot find any template for creating service account when serviceAccount.create == true. Could you validate whether you commited all the stuff?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] wiktor2200 commented on a change in pull request #17880: feat: Helm - custom service account creation and management
Posted by GitBox <gi...@apache.org>.
wiktor2200 commented on a change in pull request #17880:
URL: https://github.com/apache/superset/pull/17880#discussion_r776317258
##########
File path: helm/superset/values.yaml
##########
@@ -25,6 +25,11 @@ replicaCount: 1
# Runn containers as root is not recommended in production. Change this to another UID - e.g. 1000 to be more secure
runAsUser: 0
+# Create custom service account for Superset. If create: true and name is not provided, superset.fullname will be used.
+# serviceAccountName: superset
+serviceAccount:
+ create: false
Review comment:
If someone have blocked version on helm in their definition it's OK, but when someone is using "latest" it will brake down deployment (or dependent components).
Supporting two solutions for a while should be quite easy (I'll just add one more conditions in `_helpers`) but this could cause some caveats such as overwriting values if `serviceAccountName` and `serviceAccount.name` would be defined in the same time. :thinking:
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] przemyslawandruszewski commented on a change in pull request #17880: feat: Helm - custom service account creation and management
Posted by GitBox <gi...@apache.org>.
przemyslawandruszewski commented on a change in pull request #17880:
URL: https://github.com/apache/superset/pull/17880#discussion_r776314522
##########
File path: helm/superset/values.yaml
##########
@@ -25,6 +25,11 @@ replicaCount: 1
# Runn containers as root is not recommended in production. Change this to another UID - e.g. 1000 to be more secure
runAsUser: 0
+# Create custom service account for Superset. If create: true and name is not provided, superset.fullname will be used.
+# serviceAccountName: superset
+serviceAccount:
+ create: false
Review comment:
Yeps you are right, we would break backward compatibility but on the other site, we could maintain both for sometime and prepare a communication about that right now.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] wiktor2200 commented on pull request #17880: feat: Helm - custom service account creation and management
Posted by GitBox <gi...@apache.org>.
wiktor2200 commented on pull request #17880:
URL: https://github.com/apache/superset/pull/17880#issuecomment-1004607632
Unfortunately I can't. :( It has to be clicked by someone with write access to repo.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] amitmiran137 commented on pull request #17880: feat: Custom service account creation and management
Posted by GitBox <gi...@apache.org>.
amitmiran137 commented on pull request #17880:
URL: https://github.com/apache/superset/pull/17880#issuecomment-1002182398
Could you bump version of the chart manually ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] craig-rueda commented on a change in pull request #17880: feat: Helm - custom service account creation and management
Posted by GitBox <gi...@apache.org>.
craig-rueda commented on a change in pull request #17880:
URL: https://github.com/apache/superset/pull/17880#discussion_r777126120
##########
File path: helm/superset/values.yaml
##########
@@ -25,6 +25,11 @@ replicaCount: 1
# Runn containers as root is not recommended in production. Change this to another UID - e.g. 1000 to be more secure
runAsUser: 0
+# Create custom service account for Superset. If create: true and name is not provided, superset.fullname will be used.
+# serviceAccountName: superset
+serviceAccount:
+ create: false
Review comment:
I think it LGTM. If people are using "latest" in any env that matters, that's generally a bad practice. Using the default of `create: false` seems reasonable to me.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] wiktor2200 commented on a change in pull request #17880: feat: Helm - custom service account creation and management
Posted by GitBox <gi...@apache.org>.
wiktor2200 commented on a change in pull request #17880:
URL: https://github.com/apache/superset/pull/17880#discussion_r776671406
##########
File path: helm/superset/templates/service-account.yaml
##########
@@ -0,0 +1,15 @@
+{{- if .Values.serviceAccount.create -}}
+apiVersion: v1
+kind: ServiceAccount
+metadata:
+ name: {{ if .Values.serviceAccountName }}{{ .Values.serviceAccountName }}{{ else }}{{ include "superset.fullname" . }}{{ end }}
Review comment:
That's good idea, I've written whole conditional before working on `_helpers` :)
It's done!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] przemyslawandruszewski commented on pull request #17880: feat: Helm - custom service account creation and management
Posted by GitBox <gi...@apache.org>.
przemyslawandruszewski commented on pull request #17880:
URL: https://github.com/apache/superset/pull/17880#issuecomment-1004575073
@wiktor2200 looks like you can merge :)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] villebro merged pull request #17880: feat: Helm - custom service account creation and management
Posted by GitBox <gi...@apache.org>.
villebro merged pull request #17880:
URL: https://github.com/apache/superset/pull/17880
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] przemyslawandruszewski commented on a change in pull request #17880: feat: Helm - custom service account creation and management
Posted by GitBox <gi...@apache.org>.
przemyslawandruszewski commented on a change in pull request #17880:
URL: https://github.com/apache/superset/pull/17880#discussion_r776305810
##########
File path: helm/superset/values.yaml
##########
@@ -25,6 +25,11 @@ replicaCount: 1
# Runn containers as root is not recommended in production. Change this to another UID - e.g. 1000 to be more secure
runAsUser: 0
+# Create custom service account for Superset. If create: true and name is not provided, superset.fullname will be used.
+# serviceAccountName: superset
+serviceAccount:
+ create: false
Review comment:
How about:
1. Remove "serviceAccountName"
2. creating serviceAccount sestion as you did with two properties:
- create as you did
- name
According to: https://helm.sh/docs/chart_best_practices/rbac/#yaml-configuration
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] wiktor2200 commented on pull request #17880: feat: Helm - custom service account creation and management
Posted by GitBox <gi...@apache.org>.
wiktor2200 commented on pull request #17880:
URL: https://github.com/apache/superset/pull/17880#issuecomment-1002570386
@amitmiran137 there is something wrong with workflow. It shows error in code which I didn't change.
BTW, who can be assigned as reviewer for this PR? Are you maintainer of helm charts?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] przemyslawandruszewski commented on a change in pull request #17880: feat: Helm - custom service account creation and management
Posted by GitBox <gi...@apache.org>.
przemyslawandruszewski commented on a change in pull request #17880:
URL: https://github.com/apache/superset/pull/17880#discussion_r776362915
##########
File path: helm/superset/templates/service-account.yaml
##########
@@ -0,0 +1,15 @@
+{{- if .Values.serviceAccount.create -}}
+apiVersion: v1
+kind: ServiceAccount
+metadata:
+ name: {{ if .Values.serviceAccountName }}{{ .Values.serviceAccountName }}{{ else }}{{ include "superset.fullname" . }}{{ end }}
Review comment:
Wouldn't that be enough to use:
{{ include "superset.serviceAccountName" . }}
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] wiktor2200 commented on pull request #17880: feat: Helm - custom service account creation and management
Posted by GitBox <gi...@apache.org>.
wiktor2200 commented on pull request #17880:
URL: https://github.com/apache/superset/pull/17880#issuecomment-1002627957
@przemyslawandruszewski you got right, thanks for notifying that.
IDK how this happened, but I've not commited this file. Maybe I run git add with some regexp and it has not been added :man_shrugging:
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] wiktor2200 commented on pull request #17880: feat: Custom service account creation and management
Posted by GitBox <gi...@apache.org>.
wiktor2200 commented on pull request #17880:
URL: https://github.com/apache/superset/pull/17880#issuecomment-1002184782
Sure! It's done :)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] amitmiran137 commented on pull request #17880: feat: Helm - custom service account creation and management
Posted by GitBox <gi...@apache.org>.
amitmiran137 commented on pull request #17880:
URL: https://github.com/apache/superset/pull/17880#issuecomment-1002585655
> @amitmiran137 there is something wrong with workflow. It shows error in code which I didn't change.
> BTW, who can be assigned as reviewer for this PR? Are you maintainer of helm charts?
Yes we became aware lately
It seems to be broken also in master
We are trying to solve this
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] przemyslawandruszewski commented on pull request #17880: feat: Helm - custom service account creation and management
Posted by GitBox <gi...@apache.org>.
przemyslawandruszewski commented on pull request #17880:
URL: https://github.com/apache/superset/pull/17880#issuecomment-1002918566
The rest is fine for me, let's leave final decision for @craig-rueda :)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] wiktor2200 commented on pull request #17880: feat: Helm - custom service account creation and management
Posted by GitBox <gi...@apache.org>.
wiktor2200 commented on pull request #17880:
URL: https://github.com/apache/superset/pull/17880#issuecomment-1003929611
@craig-rueda Pipeline failed because of missing license header. I've added it, could you re-approve? :)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org