-
Notifications
You must be signed in to change notification settings - Fork 35
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
Using team namespaces: Kubeconfig Step Thru #616
Using team namespaces: Kubeconfig Step Thru #616
Conversation
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsNo conflicts as of last run. |
1dde333
to
730b203
Compare
Nice, took a brief look and this looks good! I think the prefix approach is fine, and then we can revisit in a follow up a more comprehensive approach based on labels/annotations, which would remove the need for any sort of naming convention in our namespaces. |
c92bb66
to
6f5264b
Compare
966363b
to
dc39367
Compare
Did a walk through of the commits, looks good. I think it would be helpful to squash the commits into logical groups, e.g., one commit for adding the ns option to the cli arguments, one commit for all of the namespace prefix logic, etc. I found it a bit confusing to step through each commit where later commits were re-editing lines of code that had already been touched. Alternatively, people can just review the files, but given the scope of the PR I think logical commits would be helpful. |
822f0c8
to
531e436
Compare
@josibake Updated the commit structure as you suggested. |
41c6727
to
0dbeac8
Compare
@josibake This PR is looking good I think. Added a flag to |
0dbeac8
to
fa08a48
Compare
rebased on main |
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.
ok this is a lot, i breezed through each commit and added one comment. going to focus on test and docs next
src/warnet/admin.py
Outdated
namespace: {namespace} | ||
user: {sa} | ||
current-context: {sa}-{namespace} | ||
""" |
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.
might be cleaner to implement as a dict and then use yaml.dump()
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.
Fixed here: f50dcf8 "admin: make kubeconfig a dict"
src/warnet/bitcoin.py
Outdated
if not namespace: | ||
namespace = get_default_namespace() |
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 wonder if the repetition of this can be smoothed out with a function decorator like @needs_namespace
that can be applied to all relevant commands
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.
Fixed here: 2a906e2 "DRY out the namespace check"
And use yaml.dump
`get_namespaces_by_prefix` is now renamed to `get_namespaces_by_type` in anticipation of using labels in the future.
We use this to check if the current user can delete pods.
We want to back out early if the user is not able to bring down the network
The K8sError allows us to easily handle one single error that crops up from functions that life in k8s.py. Right now I just have it implemented in the open/write kubeconfig functions.
We use this following to check for the scenario file: while [ ! -f /shared/archive.pyz ]; do echo "Waiting for /shared/archive.pyz to exist..." sleep 2 done I suspect this may cut off longer-running copy commands. Using mv we make sure all the data is copied over, and then we `mv` the file into place.
f35c0c4
to
424eba8
Compare
rebased on main |
I've created this as an extension on #598. It includes
deploy
function to create tanks in a specific namespace and "--to-all-users"down
function that asks before nuking a whole wargame (and a "--force" to not bother asking)namespaces destroy
needs to target wargames namespaces #618