-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
feat: stream output for custom workflows #2261
Conversation
@@ -0,0 +1,155 @@ | |||
package models |
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.
I don't think this is a particularly good place for this, but noticed it was where the other exec
functionality was put. Not totally sure on the naming convention of this project, but it couldn't be in runtime
because of import cycles with the terraform
package.
// Callers can use the input channel to pass stdin input to the command. | ||
// If any error is passed on the out channel, there will be no | ||
// further output (so callers are free to exit). | ||
func (s *ShellCommandRunner) RunCommandAsync(ctx command.ProjectContext) (chan<- string, <-chan Line) { |
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 code was extracted as-is from terraform_client.go
@jamengual can you clarify on the "waiting-for-response" label? Is that with regards to the ANSI stripping, or something else? |
usually PRs do not get updated same day and linger for quite a while, it is a way to track when we are waiting for something from the contributor, nothing special |
Nope, this is ready to go on my end, just waiting for review. |
Thanks, waiting eagerly for this. Any chances for a review @jamengual ? |
We're looking forward for having this feature too :) |
@ascandella I think they fixed some build bugs in the upstream, can you fetch from upstream so we can get green checkmarks on this PR? |
@SudoSpartanDan thanks for the heads up. I've merged the latest master into this branch |
I tried to work with the docker image produced by this PR plus terragrunt. Somehow I always went into lock problems but not really reproducible. |
I'm interesting in merging this to the prerelease but @frank-bee comment makes me wonder @ascandella is there anything you could assist here? @frank-bee can you describe your atlantis config and atlantis.yaml? |
I've been running this branch on my install for about a month now without issues. I haven't rebuilt/redeployed since I merged @frank-bee very curious what you mean by "lock problems". Could you provide any more detail/logs/observed behavior? |
Hi @jamengual and @ascandella , here some more info... "lock problems" atlantis repo config
atlantis server config
|
From my own experience with Atlantis / Terragrunt I run into a lot of issues with autoinit. Due to this I went with below setup. Granted I also had to account for workspaces being used so that could be part of the reason.
|
@frank-bee thanks for the info. Out of curiosity, are you saying that these problems happen only when running Atlantis from this branch? Just trying to rule things out, since it's not really clear to me how the changes here could affect the locking behavior. |
@frank-bee if there is something kindly update here. This PR missed the pre release and we would very much like to have it. |
@ascandella and @mustafa89 I have no updates here. Also cannot reproduce it at the moment because I switched to the released atlantis and do not work with terragrunt anymore. |
@jamengual can we get this into the next release? Our team would love to start using this feature |
I'm not the only one that needs to review the code, so we need to wait for someone else to review it. |
@jamengual who else can review? This PR's been sitting for a bit and I know folks have been waiting for this feature. |
I was testing this in the pre-release and it seems the same problem @frank-bee has raised happened to us. I saw multiple MRs where one of the workspaces started showing the same The default workspace at path ... is currently locked by another command that is running for this pull request. Wait until the previous command is complete and try again. to the extent I had to kill the processes in the atlantis pod. I've verified it and it's definitely a bug. This happens in specific scenarios only when Terraform has to wait for an input as far as I've seen. To reproduce: Declare a variable without a value, so Terraform waits for the input. On variable "test_variable" {
description = "Testing"
type = string
} With this change you can see the plan commands stuck when you do a |
@mustafa89 thanks for the details. Looking at the built-in terraform client, it seems to append Does it work for you if you add the same flags to your workflow? For example: workflows:
terragrunt:
plan:
steps:
- env:
name: TERRAGRUNT_TFPATH
command: 'echo "terraform${ATLANTIS_TERRAFORM_VERSION}"'
- run: terragrunt plan -input=false -no-color -out=$PLANFILE
- run: terragrunt show -no-color -json $PLANFILE > $SHOWFILE
apply:
steps:
- env:
name: TERRAGRUNT_TFPATH
command: 'echo "terraform${ATLANTIS_TERRAFORM_VERSION}"'
- run: terragrunt apply -input=false $PLANFILE -no-color If so, then this is probably more of a documentation fix than something that can be done in code, since we're running arbitrary shell commands (with the |
Good point, I guess |
With runatlantis#2261, we now open a stdin pipe like the built-in terraform client does. Doing so apparently caused some user's `plan`s to hang when they had variables requiring inputs, since `terraform` saw that stdin was open and prompted for a value.
Cool, glad that worked! Opened #2350 to update the terragrunt workflow docs |
With #2261, we now open a stdin pipe like the built-in terraform client does. Doing so apparently caused some user's `plan`s to hang when they had variables requiring inputs, since `terraform` saw that stdin was open and prompted for a value.
* Start threading job output to RunStepRunner * Strip ANSI * Fix lint * Use waitgroup to avoid test flakiness * Move waitgroup higher * Add ANSI test and use strings.Builder * Fix lint * Use errors.Wrap per style guide * Create ShellCommandRunner to encapsulate streaming * WIP: shell command runner * Update signatures to propagate error finding version * Fix log output * Fix error checking * Fix accidental whitespace stripping * Remove unused struct field * Fix error checking in terraform client * Add unit tests to verify command output handler was called * Remove err from async interface * Remove duplicative log now that shell command runner does it * Hide output in stream for env/multienv * Add comment explaining goroutines * Use printf for better macOS compatibility
With runatlantis#2261, we now open a stdin pipe like the built-in terraform client does. Doing so apparently caused some user's `plan`s to hang when they had variables requiring inputs, since `terraform` saw that stdin was open and prompted for a value.
Fixes #2054
I noticed that in #1937 which originally added this feature (e.g. https://github.com/runatlantis/atlantis/pull/1937/files#diff-edf527ba8643ff7bfca5f560491ea7055af472f5d6f3bbda127f1776b63d4b06L179) that the documentation around setting up
terragrunt
for custom workflows removed the-no-color
option.I'm not sure if this was by mistake, but to allow colorization in the in-browser terminal I've added
ansi.Strip()
for parsing all command output from the new runner abstraction. If this is out of scope I can back those changes out (but we should probably update the docs to re-add the-no-color
flags toterragrunt
workflows, otherwise the output is mangled in e.g. GitHub comments).Tested by updating my atlantis instance to a build of this branch and removing the
-no-color
flags from my workflow.I'm using the following server configuration:
Output as usual in GitHub comments (I have
--enable-diff-markdown-format
on):