-
Notifications
You must be signed in to change notification settings - Fork 2
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
Implement Refresh Worker Certificates Logic #65
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.
Thanks a lot @mateoflorido! Wonderful job! Almost LGTM, just left some minor comments and questions.
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 pass. We should document/comment well here since the flow is a bit more complex than normal.
6adb9af
to
78f1b48
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.
Amazing job! Thanks a lot @mateoflorido! LGTM, Just some very minor comments.
|
||
var seconds int | ||
|
||
eg, ctx := errgroup.WithContext(ctx) |
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.
Really liked this use of errgroup
! Amazing!
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, seems like @HomayoonAlimohammadi covered most of the comments.
2f8511d
to
5887593
Compare
LGTM @mateoflorido but as also discussed yesterday with @HomayoonAlimohammadi I'd like to have a test for this before we merge this. |
c2989e9
to
5624b82
Compare
025c319
to
cdb76e0
Compare
16d76aa
to
a84806a
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.
LGTM, amazing work @mateoflorido
Important
This pull request is dependent on
k8s-snap
#713Overview
This update implements the logic to handle certificate refresh for worker nodes.
Changes