feat: emit pod events on drain #703
Conversation
pkg/node/node_test.go
Outdated
| @@ -61,7 +63,10 @@ func TestDryRun(t *testing.T) { | |||
| tNode, err := node.New(config.Config{DryRun: true}) | |||
| h.Ok(t, err) | |||
|
|
|||
| err = tNode.CordonAndDrain(nodeName, "cordonReason") | |||
| fakeRecorder := record.NewFakeRecorder(100) | |||
There was a problem hiding this comment.
@brycahta asked https://github.com/aws/aws-node-termination-handler/pull/631/files#r868317617
- Make this const please
- Is it necessary for the buffer size to be this large?
A record.FakeRecorder object can't be constant. We could use a global var but we could incur in concurrency problems when testing, I think it's better to just instantiate a new var scoped to every unit test.
Regarding the buffer size, I lowered it to 10.
There was a problem hiding this comment.
I think what Bryan meant was that the buffer size should be a const, not the entire FakeRecorder object. Right now, 10 is a magic number used in three places, and it's hard to tell what it means without a descriptive name like RecorderBufferSize.
There was a problem hiding this comment.
Ah gotcha. Makes sense, just pushed a commit to create and use the recorderBufferSize const.
|
Tests passed locally. Is there any way to relaunch them? |
Issue #635:
Description of changes:
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
This is a followup of @dennisme 's PR #631 which in turn was a followup of @dthorsen 's work. All credits should go to them. The PR was closed for being stale with some pending questions and requests. In this PR I'll just try to satisfy these and try to get it finally merged.
I'll comment the code exactly where comments or requests were in the original PR, creating links to each original question and mentioning the author, and then I'll try to answer the question or satisfy the request.
I'm also adding some minor changes to the PR: deprecating
io/ioutilin favor ofos.