Add resource filters to config #74

svenwltr · 2018-01-26T23:19:08Z

Closes #54

Unlike proposed in #54, I let the flag named --target. I think my confusion about union and intersection came from the flag name. By naming the flag target it should be more clear how it behaves.

Also I updated the command line flag descriptions.

@rebuy-de/prp-aws-nuke Please review.

stephanlindauer · 2018-01-29T13:07:30Z

README.md

+
+If a exclude is defined in any place, then if will be ignored in any case.
+
+**Hint:** You can see all available resources types with this command:


resource types

stephanlindauer · 2018-01-29T13:07:34Z

README.md

+a resource type must be specified in all places. In other words each
+configuration limits the previous ones.
+
+If a exclude is defined in any place, then if will be ignored in any case.


an exclude
it will be ignored

svenwltr · 2018-01-30T08:21:01Z

bump

bjoernhaeuser · 2018-01-30T09:34:55Z

README.md

+release. Eventually, every resources should get deleted. You might to restrict
+which resources to delete. There are multiple ways to configure this.
+
+One way are filter, which already got mentioned. This requires to know the


bethge · 2018-01-30T13:09:35Z

pkg/types/list.go

@@ -0,0 +1,39 @@
+package types
+
+type Set []string


Have you considered using map[string]bool as the underlying data structure? Iterating over it would be less intuitive, but it would give a uniqueness guarantee. Possibly use a custom iterator function?

For initialization you could create a constructor which accepts []string. That would allow making sure that the resulting set will not have duplicate entries.

Using a map also has these two downsides:

We need a constructor, which currently isn't required (which I like).

Since our lovely golang map gets shuffled all the time, we get non-deterministic results (ie resources get nuked in a different order every time).

Valid points. It feels rather odd though to have a data structure called "set", which does not adhere to the properties of a set.
If I am not mistaken, Set{"foo", "foo"}.Remove(Set{"foo"}) would return Set{"foo"}

With the current implementation it would remove both.

I am not eager to implement something just to match a name. We can rename it, if you suggest a better name.

Oh yeah, you are totally right, it would remove both. Sorry about that.

Hmm..., naming things is hard ... how about LazySet. What do you think? It would imply it is not quite a super strict set but it certainly is a set. Feel free to stick with Set if you prefer.

"lazy" always makes me think of lazy loading. What about Collection to remove any implication from the name?

Good idea! I like it.

bethge · 2018-01-30T13:09:57Z

pkg/types/list.go

+}
+
+func (s Set) Union(o Set) Set {
+	return Set(append(s, o...))


This would create duplicate entries in the set. Do you actually need it?

bethge · 2018-01-30T13:59:42Z

pkg/types/list.go

@@ -0,0 +1,39 @@
+package types


I would probably call it set.go

bethge · 2018-01-30T15:21:14Z

config/example.yaml

@@ -4,8 +4,18 @@ regions:
 account-blacklist:
 - 1234567890

+resource-types:
+  target:


Consider having two examples, one with a blacklist and another one with a whitelist.

bethge · 2018-01-30T15:22:26Z

cmd/config.go

 	"gopkg.in/yaml.v2"
 )

+type ResourceConfig struct {
+	Targets  types.Set `yaml:"targets"`
+	Excludes types.Set `yaml:"excludes"`


Shouldn't this be yaml:"target" and yaml:"exclude" ?

IMO not, since you specify a list.

Makes sense. Doesn't this need to be targets: https://github.com/rebuy-de/aws-nuke/pull/74/files#diff-04c6e90faac2675aa89e2176d2eec7d8R223 ?

bethge · 2018-01-30T15:29:15Z

README.md

+a resource type must be specified in all places. In other words each
+configuration limits the previous ones.
+
+If an exclude is defined in any place, then it will be ignored in any case.


Consider being more explicit, along the lines of:

If an exclude is used, then all its resource types will not be deleted.

svenwltr · 2018-02-01T08:25:24Z

bump

octopusx · 2018-02-01T09:27:22Z

README.md

+### Specifying Resource Types to Delete
+
+*aws-nuke* deletes a lot of resources and there might be added more at any
+release. Eventually, every resources should get deleted. You might to restrict


"you might want to restrict" ?

svenwltr added 2 commits January 27, 2018 00:18

Add resource filters to config

203e5ae

rename target flag

b3e5e5e

svenwltr self-assigned this Jan 26, 2018

svenwltr mentioned this pull request Jan 26, 2018

Add resource filters to config #54

Closed

stephanlindauer suggested changes Jan 29, 2018

View reviewed changes

svenwltr force-pushed the improve-filters branch from eca2ce9 to 9617b75 Compare January 29, 2018 13:56

stephanlindauer previously approved these changes Jan 30, 2018

View reviewed changes

bjoernhaeuser previously approved these changes Jan 30, 2018

View reviewed changes

svenwltr dismissed stale reviews from bjoernhaeuser and stephanlindauer via c5b1926 January 30, 2018 09:39

svenwltr force-pushed the improve-filters branch from 9617b75 to c5b1926 Compare January 30, 2018 09:39

bethge reviewed Jan 30, 2018

View reviewed changes

svenwltr force-pushed the improve-filters branch from c5b1926 to 0666932 Compare January 31, 2018 08:18

octopusx reviewed Feb 1, 2018

View reviewed changes

octopusx approved these changes Feb 1, 2018

View reviewed changes

stephanlindauer approved these changes Feb 1, 2018

View reviewed changes

review fixes

4629ded

svenwltr force-pushed the improve-filters branch from 2983cd5 to 4629ded Compare February 2, 2018 14:16

bethge approved these changes Feb 3, 2018

View reviewed changes

rename Set type to Collection

88419ca

svenwltr force-pushed the improve-filters branch from e10612e to 88419ca Compare February 5, 2018 08:47

svenwltr merged commit 44434a3 into master Feb 9, 2018

svenwltr deleted the improve-filters branch February 9, 2018 16:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add resource filters to config #74

Add resource filters to config #74

svenwltr commented Jan 26, 2018

stephanlindauer Jan 29, 2018

svenwltr Jan 29, 2018

stephanlindauer Jan 29, 2018

svenwltr Jan 29, 2018

svenwltr commented Jan 30, 2018

bjoernhaeuser Jan 30, 2018

svenwltr Jan 30, 2018

bethge Jan 30, 2018

svenwltr Jan 31, 2018

bethge Feb 1, 2018

svenwltr Feb 1, 2018

bethge Feb 3, 2018

svenwltr Feb 3, 2018

bethge Feb 4, 2018

svenwltr Feb 5, 2018

bethge Jan 30, 2018

svenwltr Jan 31, 2018

bethge Jan 30, 2018

svenwltr Jan 31, 2018

bethge Jan 30, 2018

svenwltr Jan 31, 2018

bethge Jan 30, 2018

svenwltr Jan 30, 2018

bethge Jan 30, 2018

svenwltr Jan 31, 2018

bethge Jan 30, 2018

svenwltr Jan 31, 2018

svenwltr commented Feb 1, 2018

octopusx Feb 1, 2018

svenwltr Feb 1, 2018

octopusx Feb 1, 2018


		If a exclude is defined in any place, then if will be ignored in any case.

		Hint: You can see all available resources types with this command:

Add resource filters to config #74

Add resource filters to config #74

Conversation

svenwltr commented Jan 26, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

svenwltr commented Jan 30, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

svenwltr commented Feb 1, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment