search

twitchscience / kinsumer

Native Go consumer for AWS Kinesis streams.
Other
134 stars 35 forks source link

Add calls to create and delete required Dynamo tables #14

Closed brentnd closed 6 years ago

brentnd commented 6 years ago

Similar to KCL, kinsumer should provide the ability for the internal dynamo tables (_clients, _checkpoints, _metadata) to be created programmatically.

The new calls on kinsumer are CreateRequiredTables() and DeleteTables() which are opt-in so no existing functionality is changed.

No test functionality was changed but the new methods allowed a lot of existing test setup/cleanup code to be removed by using the new methods.

brentnd commented 6 years ago

Looking at the build failures. Ran golint but checking gometalinter now.

brentnd commented 6 years ago

I'm seeing these errors with and without my changes. Is it possible that gometalinter updates since the last build in May are now showing this as an issue?

statsd/statsd.go:45::warning: Errors unhandled.,LOW,HIGH (gosec)
statsd/statsd.go:46::warning: Errors unhandled.,LOW,HIGH (gosec)
statsd/statsd.go:47::warning: Errors unhandled.,LOW,HIGH (gosec)
statsd/statsd.go:48::warning: Errors unhandled.,LOW,HIGH (gosec)
statsd/statsd.go:54::warning: Errors unhandled.,LOW,HIGH (gosec)
statsd/statsd.go:55::warning: Errors unhandled.,LOW,HIGH (gosec)

The linting does pass when using gometalinter.v2.

brentnd commented 6 years ago

https://github.com/alecthomas/gometalinter/pull/505 in gometalinter renamed gas to gosec so the .travis.yml file needed to be updated to disable it by the new name.

garethlewin commented 6 years ago

FWIW the reason we didn't want to auto create the tables were so that people could setup alarms and the like. Reading code now.

brentnd commented 6 years ago

I'll take a look at the comments and incorporate the requested changes tomorrow.

brentnd commented 6 years ago

All of the change requests have been addressed.

garethlewin commented 6 years ago

@brentnd did you forget to push your changes? I do not see any new commits.

brentnd commented 6 years ago

I amended the original commit and force pushed.

brentnd commented 6 years ago

@GarethLewin Any updates on this?

garethlewin commented 6 years ago

Sorry was away all of last week.

garethlewin commented 6 years ago

Just as a general comment, please next time don't force push over your changes, makes PRs much harder to read.

brentnd commented 6 years ago

Thanks @GarethLewin. Makes sense about the force push.