-
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
Removing tf show output when streaming logs #104
Conversation
/ptal @nishkrishnan @msarvar |
/ptal #orchestration |
outCh <- Line{Line: message} | ||
c.projectCmdOutputHandler.Send(ctx, message) | ||
// Don't stream terraform show output to outCh | ||
cmds := strings.Split(tfCmd, " ") |
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.
Would it be better to actually only allow plan and apply specifically instead of just having a deny list?
Upstream has support for using terraform version
. and we might want to eventually support terraform import
.
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.
Yeah I think it makes sense to have an allow list for better control over what's being streamed to the web UI. I've added a LogStreamingValidCmds
with init
, plan
, and apply
commands for now. We can add more commands as we go :)
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, there is one change that we need to make. I left a comment
s := bufio.NewScanner(stdout) | ||
for s.Scan() { | ||
message := s.Text() | ||
outCh <- Line{Line: message} |
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.
This needs to always run. outCh
is used to display the terraform command output on the PR. isValidCommand
is only relevant for c.projectCmdOutputHandler.Send(ctx, message)
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.
True! Thanks for the catch :) I've made the change you suggested.
@Aayyush one more thing :) You need to add the same check for atlantis/server/events/terraform/terraform_client.go Lines 460 to 465 in 12b3873
|
No description provided.