-
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 good few minor comments
Would add this feature to the SDK as well so all providers can have this feature. |
I originally thought to just add it to the SDK but was concerned about adding destructive actions... wasn't sure if the directory was always 100% dynamically generated or if there could be manual additions |
We can add it as an optional flag |
@roneli - Created an issue on the cq-provider-sdk to implement this functionality |
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 two suggestions
Co-authored-by: Ron <38083777+roneli@users.noreply.github.com>
Co-authored-by: Ron <38083777+roneli@users.noreply.github.com>
Ran into issues when changing a table name where the github checks kept failing because the old table file was never being deleted. By clearing out all of the files for each run this will avoid that issue