search

99designs / iamy

A cli tool for importing and exporting AWS IAM configuration to YAML files
MIT License
238 stars 24 forks source link

Add option to exclude S3 #69

Closed wasabigeek closed 3 years ago

wasabigeek commented 4 years ago

Fixes https://github.com/99designs/iamy/issues/64.

I'm not familiar with Go so this was pretty much winging it. Appreciate some advice on the implementation (passing a param to Fetch doesn’t feel ideal to me), then I'll make the required changes.

wasabigeek commented 4 years ago

Sorry I realise this doesn't quite work the way I think it does for Push, there are some edge cases if it was synced with S3 and then later an exclude attempt was made >_<

wasabigeek commented 4 years ago

Made such that the flag is set on the Fetcher struct instead, also added it to the YamlDumper so it should work even if there was a previous S3 pull which has been desynced. Would love to hear your thoughts - I do think it’ll be nice to set a more permanent config (perhaps an environment variable or a special YAML config?) but that could be a separate enhancement.

wasabigeek commented 4 years ago

@mtibben hope I'm not being a bother, let me know if I can help this along (e.g. adding more tests). I've stopped short of moving the split up Fetchers to their own files

wasabigeek commented 4 years ago

Thanks @patrobinson! I don’t have permissions to merge this, is this still pending something?

patrobinson commented 4 years ago

I want to do some local testing and cut a release, so it's pending me for now.

wasabigeek commented 4 years ago

@patrobinson is this still something you'd like to include? I can try to clean it up and resolve conflicts if so :)

patrobinson commented 3 years ago

@wasabigeek sorry for taking so long to get back to you. If you could resolve the conflicts I will take a look at this.

vquangna2 commented 3 years ago

are this feature ready?

wasabigeek commented 3 years ago

@mtibben @patrobinson am trying to set aside some time to look at this again, wanted to get your thoughts on implementation.

In this PR I attempted to split some of the AwsFetcher logic into AwsAccountFetcher, AwsIamFetcher, AwsS3Fetcher, mostly so they could be mocked in tests. Now that https://github.com/99designs/iamy/pull/72 is merged, I'm also thinking of extracting the isSkippableManagedResource logic into an AwsSkippableResourceChecker, which will be injected into the previously mentioned classes. How does that sound?

Also, were there any concerns previously that prevented this from being merged in?