Initial version of tag-based filterring #237
Conversation
.gitignore
Outdated
| @@ -2,3 +2,5 @@ | |||
| /aws-nuke | |||
| /aws-nuke-* | |||
| .glide | |||
| .idea | |||
There was a problem hiding this comment.
We do not want to add any IDE specific ignores here. Every developer should use any IDE or text editor they want and requiring to update our .gitignore to reflect the use of all the quirky files some IDEs produce would be a cumbersome tasks.
You rather should configure a global gitignore.
resources/cloudformation-stack.go
Outdated
| @@ -3,7 +3,8 @@ package resources | |||
| import ( | |||
| "github.com/aws/aws-sdk-go/aws/session" | |||
| "github.com/aws/aws-sdk-go/service/cloudformation" | |||
| ) | |||
| "github.com/rebuy-de/aws-nuke/pkg/types" | |||
| ) | |||
There was a problem hiding this comment.
The indentation is wrong. Are you using gofmt?
resources/cloudformation-stack.go
Outdated
| }) | ||
| return err | ||
| } | ||
|
|
||
| func (cfs *CloudFormationStack) Properties() types.Properties { | ||
| properties := types.NewProperties() | ||
| properties. |
There was a problem hiding this comment.
Formatting looks also wrong here.
There was a problem hiding this comment.
I would remove the newline here.
resources/cloudformation-stack.go
Outdated
| properties. | ||
| Set("Name", cfs.stack.StackName) | ||
| for _, tagValue := range cfs.stack.Tags { | ||
| properties.Set("tag:"+*tagValue.Key, tagValue.Value); |
There was a problem hiding this comment.
I would create a new function in properties for this. Something like properties.SetTag(tagValue.Key, tagValue.Value). This function can then add the tag: prefix.
- We can do proper
nilchecks inSetTag. IfKeyisnilfor some reason the scan will terminate for this resource. - We avoid typos for the prefix.
- We get a proper types interface in
properties. With this we are able to do more sophisticated stuff inside ofPropertieswithout changing every resource. Otherwise we would have to do a prefix check for every property to implement the global include filter.
There was a problem hiding this comment.
The docs say it is *string and not string. If it is nil the dereference *tagValue.Key would cause a panic.
|
@svenwltr thanks for feedback, see update. |
resources/cloudformation-stack.go
Outdated
| }) | ||
| return err | ||
| } | ||
|
|
||
| func (cfs *CloudFormationStack) Properties() types.Properties { | ||
| properties := types.NewProperties() | ||
| properties. |
There was a problem hiding this comment.
I would remove the newline here.
|
@nisabek Thank you very much for your contribution. We can now start to add this to other resources and once we have a good coverage we can start to think about the global include feature. |
svenwltr
added
the
kind/enhancement
@svenwltr - this is what I had in mind. Not sure we need an extra Map for tags...
let me know what you think. @leflamm interesting for you too :)