-
Notifications
You must be signed in to change notification settings - Fork 13
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 refresh-certs
Command
#623
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.
Overrall, seems pretty straight forward. I do like the human parsing of the date on when to refresh next
src/k8s/cmd/k8s/k8s_refresh_certs.go
Outdated
ttl, err := utils.TTLToSeconds(opts.ttl) | ||
if err != nil { | ||
cmd.PrintErrf("Error: Failed to parse TTL. \n\nThe error was: %v\n", err) | ||
} | ||
|
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.
❤️ Super clear
if err != nil { | ||
cmd.PrintErrf("Error: Failed to create a k8sd client. Make sure that the k8sd service is running.\n\nThe error was: %v\n", err) | ||
env.Exit(1) | ||
return | ||
} |
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.
Nice, maybe the k8sd
service isn't running. If that's the only reason, maybe indicate how to check that.
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.
Great work, did an initial round.
6dfff05
to
5c97885
Compare
44fd322
to
1f087c6
Compare
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.
Nice, LGTM. thanks for addressing my questions @mateoflorido
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.
LGTM, great work!
Overview
Add a
refresh-certs
command to refresh certificates for both control plane and worker nodes.Rationale
In Proposal 002, we outlined the process for refreshing certificates via the
k8s
command line interface. This allows the snap to refresh certificates for both control plane and worker nodes in the cluster.Testing
Discussion
The
RefreshCertificatesRunResponse
contains the certificate validity period in seconds. However, parsing this from the CLI might show the user an inaccurate expiration date. Therefore, it may be better to return the UNIX timestamp, which would allow the actual date to be encoded in that field.