-
Notifications
You must be signed in to change notification settings - Fork 41
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 create-kubeconfigs command #545
add create-kubeconfigs command #545
Conversation
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.
+1 for replacing another bash script! LGTM, I think we can merge as is in the interest of time, but would be nice to leave some TODO hints for follow ups.
src/warnet/admin.py
Outdated
@@ -1,4 +1,5 @@ | |||
import os | |||
import subprocess |
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.
do we need subprocess? iirc, we have a warnet command runner (run_command, or stream_command?)
|
||
|
||
# Get all namespaces that start with "warnet-" | ||
def get_warnet_namespaces(): |
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.
if we are going to start having conventions around namespace strings (e.g., warnet-), might be more useful to make this a "get_namespaces_by_prefix(prefix: str)" function. not urgent, though
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.
I agree with @josibake but more strongly. I think we should avoid using any naming conventions like we did before helm (if possible) -- can we use kubernetes metadata "labels" instead?
# Create a kubeconfig file for the user | ||
kubeconfig_file = os.path.join(kubeconfig_dir, f"{sa}-{namespace}-kubeconfig") | ||
|
||
kubeconfig_content = f"""apiVersion: v1 |
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.
works for now, but ideally we keep the k8s yamls out of the python code. maybe just leave a #TODO comment to move this at some point ?
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first. |
Users need access to a number of resources so that they can run scenarios.
I transmuted the setup_user_contexts.sh script into python code. I also made it so that it would query all namespaces that start "warnet-" looking for service accounts, and then create token credentials for those accounts. Currently, user namespaces must start with "warnet-".
3d223cb
to
1d583d0
Compare
I added your comments as TODOs |
Pretty sure I understand what this is doing but it would be nice to have a test or docs to read so I know who to try it out myself, especially this week with our pre-tabconf trials |
I'll take these tomorrow and 1) add the changes I proposed in #543 and 2) add a README on how to deploy namespaces for multiple teams. As an fyi, this all works today, these PRs are just setting/documenting sane defaults in the yaml files, so should be a very easy change. |
will rebase this on master now that #543 is merged and address the outstanding comments from @pinheadmz and @me |
This is continued in #598 |
I transmuted the setup_user_contexts.sh script into python code.
I also made it so that it would query all namespaces that start "warnet-" looking for service accounts,
and then create token credentials for those accounts.
Currently, user namespaces must start with "warnet-".
Also, I included the commits from #544 and #543 because they are necessary for this.