Skip to content
This repository has been archived by the owner on Oct 15, 2024. It is now read-only.

Register MSK service #359

Merged
merged 1 commit into from
Apr 30, 2019
Merged

Register MSK service #359

merged 1 commit into from
Apr 30, 2019

Conversation

guillermo-menjivar
Copy link
Contributor

@guillermo-menjivar guillermo-menjivar commented Apr 27, 2019 •
edited
Loading

An attempt to register MSK Service requested in issue #354

rabbbithole$ docker run -a stdin -a stdout -ti rebuy-de/aws_nuke -c config/example.yaml --secret-access-key=$AWS_SECRET_ACCESS_KEY --access-key-id=$AWS_ACCESS_KEY_ID
aws-nuke version v2.10.0.4.g6b3bdb7.dirty - Sat Apr 27 07:01:51 UTC 2019 - 6b3bdb7c2bc4dfb65b08ec404983539b059fe243

Do you really want to nuke the account with the ID 12344123445 and the alias 'test-account'?
Do you want to continue? Enter account alias to continue.
> test-account

us-east-2 - MSKCluster - arn:aws:kafka:us-east-2:12344123445:cluster/test-cluster/4a914bb3-cadd-4e2c-b9d8-ad8166d381ff-3 - [Arn: "arn:aws:kafka:us-east-2:12344123445:cluster/test-cluster/4a914bb3-cadd-4e2c-b9d8-ad8166d381ff-3", Name: "test-cluster"] - would remove
Scan complete: 1 total, 1 nukeable, 0 filtered.

The above resources would be deleted with the supplied configuration. Provide --no-dry-run to actually destroy resources.

@guillermo-menjivar guillermo-menjivar requested a review from a team April 27, 2019 07:12
svenwltr
svenwltr reviewed Apr 29, 2019
View reviewed changes
resources/msk-cluster.go Outdated

func (m *MSKCluster) Properties() types.Properties {
properties := types.NewProperties()
properties.Set("Arn", m.arn)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
properties.Set("Arn", m.arn)
properties.Set("ARN", m.arn)

I would prefer upper case here, because the golang conventions recommend them (https://github.com/golang/go/wiki/CodeReviewComments#initialisms) and I would like to follow them as much as possible.

@guillermo-menjivar
Copy link
Contributor Author

Sounds good @svenwltr. I will amend the PR and make sure future PR follow the spec. Thanks

@guillermo-menjivar guillermo-menjivar mentioned this pull request Apr 29, 2019
Closed
@guillermo-menjivar
Copy link
Contributor Author

@svenwltr I have amended the PR based on your feedback

@svenwltr
Copy link
Member

Hey @videte47. Sorry for not being clear on this. The upper case writing is only meant of acronyms. ARN is an acronym (Amazon Resource Name), but name isn't. The other property should still be written as Name (Upper Camel Case).

@guillermo-menjivar
Copy link
Contributor Author

Lol,sorry that is my fault 🙃. Let me fix it - take 3...

@guillermo-menjivar
Copy link
Contributor Author

@svenwltr whenever you have time I have updated thanks !

@svenwltr svenwltr merged commit 2158279 into rebuy-de:master Apr 30, 2019
@svenwltr
Copy link
Member

Thank you!

@svenwltr svenwltr added the kind/resource Adding or changing AWS resources. label May 20, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@svenwltr svenwltr svenwltr left review comments

@bjoernhaeuser bjoernhaeuser bjoernhaeuser approved these changes

Assignees
No one assigned
Labels
kind/resource Adding or changing AWS resources.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@guillermo-menjivar @svenwltr @bjoernhaeuser