-
Notifications
You must be signed in to change notification settings - Fork 47
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 keystone-shell container #185
base: master
Are you sure you want to change the base?
Conversation
The keystone-shell is a container to help debug keystone instances in Kubernetes. It provides a ptipython-based shell with highlighting, autocomplete, and access to the Keystone and Kubernetes APIs, along with an OpenStack client and utility function to read and swap between credentials from secrets (in any namespace) at runtime.
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.
Some small suggestions for code, rest looks fine for me (take into account I still have no knowledge about Kubernetes).
keystone-shell/README.md
Outdated
|
||
| Variable | Default | Description | | ||
|--------------------|------------------|---------------------------------| | ||
| `LOG_LEVEL` | `INFO` | MySQL server host | |
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.
Wrong descriptions in many lines.
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.
wow, looks like my copy&pasting skills are getting rusty!
if e.response.status_code != 404: | ||
raise | ||
|
||
return None |
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 think this return should have one less indentation level. I know you run it from except
but this way you will make it default.
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.
Not that it matters a ton since the code paths are effectively equivalent but my feeling was that (without the conditional to check for a 404) is a bit more clear about the intent.
Ideally it would've been:
try:
return client.get(...)
except NotFound:
return None
... which I think makes the intent more clear - that is, None
isn't the "default" branch so much as the "not found" branch. Since requests doesn't have exception subclasses for different error codes, it gets a bit uglier when we compare the error code ... I'm not sure how much more readable the end result is, though.
|
||
# purge existing keystone vars from the environment | ||
for var in keystone_env_vars().keys(): | ||
print('unset {};'.format(var)) |
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.
Should have some printed info that user need to use it.
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.
Oh, you run this file output with eval
.
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.
yup, the user won't ever run this script directly, the bash alias will take care of evaling the output for them.
if __name__ == '__main__': | ||
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.
Too many empty lines at the end of file.
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.
good catch, fixed
|
||
logger.info('now using account from secret %s', secret_name) | ||
else: | ||
logger.info('no secret name arg provided, will use default environment') |
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.
Maybe you could move it up with reversed if len(sys.argv) > 1:
to have it close to the place why did it run and not with just bare else
after a lot of code.
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 think you're right, it's more clear to write that a bit differently.
keystone-shell/Dockerfile
Outdated
@@ -0,0 +1,23 @@ | |||
FROM python:3.6-alpine3.6 |
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.
Can't we just use the monasca/python.
It should have all musl and stack-fix included already AFAIR.
keystone-shell/Dockerfile
Outdated
apk add --no-cache --virtual build-dep \ | ||
musl-dev linux-headers git make g++ libffi-dev openssl-dev && \ | ||
gcc -shared -fPIC /stack-fix.c -o /stack-fix.so && \ | ||
pip install urllib3 ipaddress ipython ptpython python-openstackclient && \ |
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.
With monasca/python we could always install latest master, but I am not really sure if that's needed at all ?
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.
Yeah, you're right, this should really be using monasca/python. The docker stuff is mostly a copy&paste hack job that came out of my frustration while debugging keystone-init.
I'm fine with holding off on merging until it's converted, though.
I have really bad and nasty problems trying to make it run. [Errno 2] No such file or directory: '/usr/local/lib/python3.6/site-packages/ptyprocess-0.5.2.dist-info/top_level.txt' From within the container I called ```openstack token issue```` |
Monasca/python changes are merged so this change could be updated to reflect that. |
Also include a monasca client and improve docs.
Signed-off-by: Tim Buckley <[email protected]>
Signed-off-by: Tim Buckley <[email protected]>
The keystone-shell is a container to help debug keystone instances in Kubernetes. It provides a ptipython-based shell with highlighting, autocomplete, and access to the Keystone and Kubernetes APIs, along with an OpenStack client and utility function to read and swap between credentials from secrets (in any namespace) at runtime.