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 possibility to use configured Service Account Name for init job. #17888

Conversation

przemyslawandruszewski
Copy link

@przemyslawandruszewski przemyslawandruszewski commented Dec 29, 2021

SUMMARY

Possibility to use Service Account Name by init-job.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

helm install superset .

ADDITIONAL INFORMATION

  • Has associated issue:
  • Required feature flags:
  • Changes UI
  • Includes DB Migration (follow approval process in SIP-59)
    • 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

@przemyslawandruszewski
Copy link
Author

Hi @craig-rueda! Could you please review?

@wiktor2200
Copy link
Contributor

Could you please take a look on my PR and issue?
PR: #17880
issue: #17879
I've extended this feature (added service account creation if needed and added helpers for managing multiple conditions).
If you want, I can add your PR to mine, so custom service account will be available everywhere.

@przemyslawandruszewski
Copy link
Author

Hey @wiktor2200, Sure I have just quickly looked at your PR so it looks nice, I will have maybe 1/2 suggestions so if you are OK, let's merge the changes? :)

@wiktor2200
Copy link
Contributor

Ok, so I will add your changes to my PR. Sure feel free to add your suggestions, all feedback is welcome :)

@amitmiran137
Copy link
Member

Ok, so I will add your changes to my PR. Sure feel free to add your suggestions, all feedback is welcome :)

after this one: #17886 is merged. so rebase from master and get that fix in

@@ -31,6 +31,9 @@ spec:
{{ toYaml .Values.init.podAnnotations | nindent 8 }}
{{- end }}
spec:
{{- if .Values.serviceAccountName }}
Copy link
Member

Choose a reason for hiding this comment

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

Can you add a default value, with a comment around it in the values.yaml?

@przemyslawandruszewski
Copy link
Author

I can see that @wiktor2200 got his PR #17880 merged so I will close this one. Thanks a lot for review!

@wiktor2200
Copy link
Contributor

wiktor2200 commented Jan 4, 2022

Thanks a lot for help and co-op!
I've missed typo in my PR, already fixed it in #17920 (wating for CR and merge). I'm mentioning it here to inform you on current progress. :)

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

Successfully merging this pull request may close these issues.

4 participants