Skip to content
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

feat: add kinesis stream support #304

Merged

Conversation

strongishllama
Copy link
Contributor

Description

This pull request adds support for nuking Kinesis Stream as mentioned in #301. I'll bring in Kinesis Firehose support in another pull request when I get a bit more time.

I pretty much just translated the work done in this pull request and applied it to Kinesis Streams so please let me know if anything is incorrect style/logic wise 😄

Tests in the kinesis_stream_test.go are passing as well as when you run the CLI with go run main.go aws --resource-type kinesis-stream.

Docs have been updated to show the Kinesis Stream support.

Because this is adding support for a new resource this pull request will introduce breaking changes as per here

@strongishllama strongishllama marked this pull request as ready for review May 26, 2022 22:59
@strongishllama
Copy link
Contributor Author

strongishllama commented May 26, 2022

My apologies, I've just had a closer look at the diff and it seems that when I saved in my IDE gofumpt has made a couple of unintended style changes in aws/aws.go, please let me know if you'd like them reverted!

}
}

func assertKinesisStreamsDeleted(t *testing.T, svc *kinesis.Kinesis, identifiers []*string) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Function assertKinesisStreamsDeleted is not invoked in any other place

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, good catch! Added the missing calls 👍

@strongishllama strongishllama force-pushed the feat-add-kinesis-stream-support branch from 90bae70 to 34406db Compare June 10, 2022 05:34
@strongishllama strongishllama requested a review from denis256 June 10, 2022 05:35
@strongishllama strongishllama force-pushed the feat-add-kinesis-stream-support branch from 34406db to d378526 Compare June 10, 2022 05:39
@strongishllama
Copy link
Contributor Author

@denis256 👋 Just checking in to see if you've had a chance to look at this yet? 😄

@denis256
Copy link
Member

denis256 commented Jul 5, 2022

Hi,
will trigger a CI build to see if all tests pass

@denis256
Copy link
Member

denis256 commented Jul 5, 2022

Failed tests:

=== CONT  TestNukeKinesisStreamOne
    kinesis_stream_test.go:116: 
        	Error Trace:	kinesis_stream_test.go:116
        	            				kinesis_stream_test.go:57
        	Error:      	Expected value not to be nil.
        	Test:       	TestNukeKinesisStreamOne
markers (0 deletion markers) from bucket cloud-nuke-test-p9bgvlvn55pl"
--- FAIL: TestNukeKinesisStreamOne (15.79s)

=== CONT  TestNukeKinesisStreamMoreThanOne
    kinesis_stream_test.go:116: 
        	Error Trace:	kinesis_stream_test.go:116
        	            				kinesis_stream_test.go:83
        	Error:      	Expected value not to be nil.
        	Test:       	TestNukeKinesisStreamMoreThanOne
--- FAIL: TestNukeKinesisStreamMoreThanOne (48.14s)

@strongishllama strongishllama force-pushed the feat-add-kinesis-stream-support branch from d378526 to d9b6ae7 Compare July 5, 2022 10:52
@strongishllama
Copy link
Contributor Author

@denis256 My bad, small logic error. Should be good to go now.

@denis256
Copy link
Member

denis256 commented Jul 7, 2022

Hi,
after re-run tests seems to pass, however appeared conflicts on files:

README.md
aws/aws.go

@strongishllama strongishllama force-pushed the feat-add-kinesis-stream-support branch from d9b6ae7 to 79da0e6 Compare July 7, 2022 23:29
@strongishllama
Copy link
Contributor Author

@denis256 Conflicts have been fixed 👍

@strongishllama strongishllama force-pushed the feat-add-kinesis-stream-support branch from 79da0e6 to 830be22 Compare July 15, 2022 00:02
@strongishllama
Copy link
Contributor Author

@denis256 Sorry to keep nagging but do you think this is good to go now? 😅

@strongishllama strongishllama force-pushed the feat-add-kinesis-stream-support branch from 830be22 to 74cdf73 Compare July 17, 2022 22:01
@denis256
Copy link
Member

Looks like after merge build is failing:

Stderr: go: downloading github.com/konsorten/go-windows-terminal-sequences v1.0.3
# github.com/gruntwork-io/cloud-nuke/aws
aws/aws.go:724:40: use of package session without selector

@strongishllama strongishllama force-pushed the feat-add-kinesis-stream-support branch from 74cdf73 to be3f5bb Compare July 18, 2022 23:20
@strongishllama
Copy link
Contributor Author

Looks like after merge build is failing:

Stderr: go: downloading github.com/konsorten/go-windows-terminal-sequences v1.0.3
# github.com/gruntwork-io/cloud-nuke/aws
aws/aws.go:724:40: use of package session without selector

Fixed 👍

@denis256
Copy link
Member

Hi,
looks like with recent changes are conflicting files:

README.md
aws/aws.go
config/config.go
config/config_test.go

@strongishllama
Copy link
Contributor Author

Hi, looks like with recent changes are conflicting files:

README.md
aws/aws.go
config/config.go
config/config_test.go

Resolved 👍

Copy link
Member

@denis256 denis256 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CI job passed without failures, going to merge into master

@denis256 denis256 merged commit 3044692 into gruntwork-io:master Jul 21, 2022
@strongishllama strongishllama deleted the feat-add-kinesis-stream-support branch July 21, 2022 22:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants