Add option to exclude S3 #69
Conversation
|
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 >_< |
|
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. |
| @@ -38,6 +39,7 @@ func main() { | |||
| pushDir = push.Flag("dir", "The directory to load yaml files from").Default(defaultDir).Short('d').ExistingDir() | |||
| ) | |||
| dryRun = kingpin.Flag("dry-run", "Show what would happen, but don't prompt to do it").Bool() | |||
| excludeS3 = kingpin.Flag("exclude-s3", "Exclude S3 policies from pull or push").Bool() | |||
There was a problem hiding this comment.
Aside from this PR, I'd love there to be a config file we could use to set these per account/repository.
iamy/aws.go
Outdated
| // Wrapper around getAccount method to make mocking easier | ||
| var getAccount = func (a *AwsFetcher) (*Account, error) { | ||
| return a.getAccount() | ||
| } |
There was a problem hiding this comment.
Using Globals to facilitate mocking isn't a very idiomatic way to do this.
How I'd approach it is to create an interface
type AwsFetcherIface interface {
GetAccount() (*Account, error)
FetchIAMData() error
FetchS3Data() error
}Then instantiate AwsFetcher and pass it into the function being tested. That way we can define our own implementation in the test suite and pass it in to the function.
There was a problem hiding this comment.
Got it! Will probably steal your work in #58 - I tried to do it but didn't grok interfaces well and ended up with a very long declaration.
There was a problem hiding this comment.
Struggling a little - the function under test is a method of AwsFetcher as well (AwsFetcher's Fetch(), which is used by push/pull, will call getAccount(), fetchIamData() and fetchS3Data methods). My limited understanding is that in creating an interface for AwsFetcher, I would also be mocking Fetch, which makes the test meaningless.
Do you have any advice? I'm deliberating:
- creating interfaces for S3/IAM clients and mocking those before passing into Fetcher - it's quite tedious though, there's potentially a lot to mock when all I want to do is check if fetchS3Data / fetchIamData is being called
- creating a separate type to hold the 3 methods and embedding that into Fetcher (something like this), but it feels a little contrived
EDIT: decided I'd try breaking apart Fetcher. A bit tricky due to the lack of tests, I did try running manually, seemed to work.
There was a problem hiding this comment.
I think breaking it up is a great idea, but also would like @mtibben to weigh in.
|
@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 |
|
Thanks @patrobinson! I don’t have permissions to merge this, is this still pending something? |
|
I want to do some local testing and cut a release, so it's pending me for now. |
|
@patrobinson is this still something you'd like to include? I can try to clean it up and resolve conflicts if so :) |
|
@wasabigeek sorry for taking so long to get back to you. If you could resolve the conflicts I will take a look at this. |
|
are this feature ready? |
|
@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 Also, were there any concerns previously that prevented this from being merged in? |
Fixes #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.