-
Notifications
You must be signed in to change notification settings - Fork 83
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
CLI and README updates #76
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.
My review is just for the README. I think there are a few changes needed.
README.md
Outdated
--astra-token <astra-token> --astra-database-id <astra-datbase-id> | ||
``` | ||
|
||
The proxy also support using the Astra Secure Connect Bundle, but it requires mounting the bundle to a volume in the container. |
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.
You need to put in a link for the scb - how to get it.
Co-authored-by: Lorina Poland <[email protected]>
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.
Just added 1 comment but not important.
proxycore/connpool.go
Outdated
@@ -209,6 +211,7 @@ func (p *connPool) stayConnected(idx int) { | |||
done = true | |||
_ = conn.Close() | |||
case <-conn.IsClosed(): | |||
p.logger.Warn("pool closed", zap.Stringer("host", p.config.Endpoint)) |
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'm not sure if Warn
is the best log level here. In Astra coordinator topology changes are somewhat common (tenant allocation/deallocation, upgrades) so it might "scare" users without a good reason when they eventually see these warnings.
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 call.
A small refactor of the cli flags. I've added an
astra
prefix to the Astra specific options e.g.--astra-bundle
(ASTRA_BUNDLE
for the env. variable).I've also update the README for the updated options including using an Astra Token (instead of the secure connect bundle).
I haven't update the k8s instruction, but I've created a follow up ticket: #77. The main reason for doing this is I'm not a fan of the current instructions because they should be putting the client secrets in the secret store instead of the command. I need to run through a setup where I use the new astra token options with the k8s secrets.