-
Notifications
You must be signed in to change notification settings - Fork 6
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Support pre-workflow hooks on all comment/auto triggered commands. #43
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.
lgtm couple minor comments.
autoPlanRunner := buildCommentCommandRunner(c, models.PlanCommand) | ||
if autoPlanRunner == nil { | ||
ctx.Log.Err("invalid autoplan command") | ||
return |
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.
Don't you want this? Otherwise go will panic.
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.
oh i think this can be removed since we panic if we can't get it from the map in the function itself:
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.
Do we even need this check then?
WorkingDirLocker: workingDirLocker, | ||
WorkingDir: workingDir, | ||
Drainer: drainer, | ||
PreWorkflowHookRunner: &runtime.PreWorkflowHookRunner{}, | ||
PreWorkflowHookRunner: runtime.DefaultPreWorkflowHookRunner{}, |
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.
Should this be a pointer object?
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.
My assumption was that you only need pointers if you have a need to reference the underlying data without copying it but this struct is stateless.
For our usecase, having this on PR open/update is not good enough since
atlantis unlock
for example cleans up the directory and each comment command attempts to clone the directory. Since we are dynamically generating the atlantis.yaml this breaks integrations until a PR event occurs again.TODO: