-
Notifications
You must be signed in to change notification settings - Fork 40
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great, I love the ConfigExist
attribute, this will make queries much more simpler.
Only mock and e2e tests are missing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, had one suggestion + as @yevgenypats we should add e2e + mock tests
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, added one suggestion to make the flow cleaner, I think you can also add account id to the initialization and remove initialization in lines 69-71.
Still missing mock tests for sanity + e2e tests I think no terraform is required, just a fetching the data and verifying values worked.
fixes: #282