Follow the next token when pages of results returned

[mtibben]

The AWS SDK Pages functions iterate over pages of results automatically using the NextToken returned from AWS API. However in order to allow the function to iterate correctly, you need to return true when NextToken exists - i.e. when the lastPage has not been reached

From the docs

To stop iterating, return false from the fn function

This change corrects code using Pages functions when the lastPage has not been reached.

[pda]

Thanks! I'll fix up whatever is wrong with the CI pipeline to get a green build then merge.

[mtibben]

Nice one, thanks @pda. We have another urgent fix at 99designs#1

[pda]

Hmm… when I run the tests locally, runner/cloudwatch_test.go is spinning for 10 minutes before timing out.
Do you think that's related to the other fix you linked to?

2021/05/13 14:50:57 Waiting for log stream my-stream to exist...
2021/05/13 14:50:58 Timed out waiting for stream
2021/05/13 14:50:58 Waiting for log stream my-stream to exist...
2021/05/13 14:50:58 Found stream my-stream after 41.58351ms
2021/05/13 14:50:58 Printing events in stream "my-stream" after 0
panic: test timed out after 10m0s

goroutine 8 [running]:
testing.(*M).startAlarm.func1()
        /usr/local/Cellar/go/1.16.3/libexec/src/testing/testing.go:1700 +0xe5
created by time.goFunc
        /usr/local/Cellar/go/1.16.3/libexec/src/time/sleep.go:180 +0x45

goroutine 1 [chan receive, 9 minutes]:
testing.(*T).Run(0xc000296000, 0x127154d, 0x24, 0x127b498, 0x1086801)
        /usr/local/Cellar/go/1.16.3/libexec/src/testing/testing.go:1239 +0x2da
testing.runTests.func1(0xc000001b00)
        /usr/local/Cellar/go/1.16.3/libexec/src/testing/testing.go:1511 +0x78
testing.tRunner(0xc000001b00, 0xc000165de0)
        /usr/local/Cellar/go/1.16.3/libexec/src/testing/testing.go:1193 +0xef
testing.runTests(0xc00000e228, 0x13ed500, 0x7, 0x7, 0xc01f4a8278e834f0, 0x8bb2e8ec91, 0x13f3220, 0x12696b3)
        /usr/local/Cellar/go/1.16.3/libexec/src/testing/testing.go:1509 +0x2fe
testing.(*M).Run(0xc000166100, 0x0)
        /usr/local/Cellar/go/1.16.3/libexec/src/testing/testing.go:1417 +0x1eb
main.main()
        _testmain.go:55 +0x138

goroutine 34 [runnable]:
github.com/buildkite/ecs-run-task/runner.(*logWatcher).printEventsAfter.func1(0xc000561a40, 0xc000561a01, 0x0)
        /Users/pda/bk/ecs-run-task/runner/cloudwatch.go:204 +0x11a
github.com/buildkite/ecs-run-task/runner.(*mockCloudWatchLogs).FilterLogEventsPages(0xc0002a0000, 0xc0002a2120, 0xc000288120, 0xc0002a8018, 0x2)
        /Users/pda/bk/ecs-run-task/runner/cloudwatch_test.go:191 +0x105
github.com/buildkite/ecs-run-task/runner.(*logWatcher).printEventsAfter(0xc0002a2000, 0x12cf510, 0xc0000160b8, 0x0, 0x3, 0x1, 0x2)
        /Users/pda/bk/ecs-run-task/runner/cloudwatch.go:203 +0x336
github.com/buildkite/ecs-run-task/runner.(*logWatcher).Watch(0xc0002a2000, 0x12cf510, 0xc0000160b8, 0x0, 0xec28be770)
        /Users/pda/bk/ecs-run-task/runner/cloudwatch.go:167 +0x28f
github.com/buildkite/ecs-run-task/runner.TestLogsWatcherWaitsForStreamToStart(0xc000296000)
        /Users/pda/bk/ecs-run-task/runner/cloudwatch_test.go:79 +0x278
testing.tRunner(0xc000296000, 0x127b498)
        /usr/local/Cellar/go/1.16.3/libexec/src/testing/testing.go:1193 +0xef
created by testing.(*T).Run
        /usr/local/Cellar/go/1.16.3/libexec/src/testing/testing.go:1238 +0x2b3

goroutine 37 [chan send, 9 minutes]:
github.com/buildkite/ecs-run-task/runner.(*logWaiter).Wait.func1(0x2faf080, 0xc000294120)
        /Users/pda/bk/ecs-run-task/runner/cloudwatch.go:86 +0x45
created by github.com/buildkite/ecs-run-task/runner.(*logWaiter).Wait
        /Users/pda/bk/ecs-run-task/runner/cloudwatch.go:84 +0x19a
FAIL    github.com/buildkite/ecs-run-task/runner        600.341s
FAIL

[pda]

Nope… cherry-picking 73c315f onto this branch doesn't fix it. And main is good. Looks like it's specific to this branch. Either the test was brittle and needs updating to match the code, or there's something wrong with the pagination change.

[pda]

TestLogsWatcherWaitsForStreamToStart is the one that's hanging.

[pda]

Ah, so I think the pagination callback boolean was flipped in the implementation (which this PR fixed) and in the test (which wasn't fixed)?

This gets the tests green:

--- a/runner/cloudwatch_test.go
+++ b/runner/cloudwatch_test.go
@@ -188,7 +188,7 @@ func (cw *mockCloudWatchLogs) FilterLogEventsPages(input *cloudwatchlogs.FilterL
                        output.Events = append(output.Events, e)
                }

-               if fn(output, len(cw.filterLogEvents) == 0) {
+               if !fn(output, len(cw.filterLogEvents) == 0) {
                        cw.Unlock()
                        return nil
                }

I'm not sure it's the ideal solution.

[pda]

Green build was in https://buildkite.com/buildkite/ecs-run-task/builds/14