ENG-13394: Add Dmap Scanner library #1
Conversation
There was a problem hiding this comment.
Thanks Victor. I have not looked at the code in detail but have given some ideas for high level structure - mainly to make the API extensible to other cloud types in the future without necessarily breaking the API. Please let me know what you think. Happy to discuss more in person.
|
I still need to add descriptions to public types and interfaces, aside from that, the PR should be in a good state for re-review. |
There was a problem hiding this comment.
LGTM and much cleaner than before, thanks @VictorGFM
I'll also let @ccampo133 take another look and see if he has any further suggestions before the PR is approved and merged.
There was a problem hiding this comment.
LGTM @VictorGFM. Just a couple comments for your consideration, but I've still approved the PR.
| err, | ||
| )) | ||
| } | ||
| for _, cluster := range rdsClusters { |
There was a problem hiding this comment.
Will rdsClusters be populated even if err is non-nil? If not, maybe we can continue when we encounter an error. Same thing for all other functions, as they follow the same pattern.
There was a problem hiding this comment.
I see, in this case returning earlier would just add a duplicated return block, because in practice when the rdsClusters is nil, it will skip the for loop and just return right after, which would be equivalent but with just a single return block.
There was a problem hiding this comment.
Sure, I don't have a strong opinion. I just personally prefer exiting early in an error state, but if rdsClusters will just be an empty slice then it's pretty much the same
Description of the change
ENG-13394: Scanner algorithm implementation
Add Dmap Scanner library implementation:
ConfigScanner ManagerimplementationRepositorymodelType of change
Checklists
Development
Code review
Testing
Tested E2E using an IAM Role for the AWS
Developmentaccount. Also tested with unit tests.