Skip to content
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

Add function for docker compose to return stdout #665

Merged
merged 10 commits into from
Oct 2, 2020

Conversation

rhoboat
Copy link
Contributor

@rhoboat rhoboat commented Oct 1, 2020

What else does this PR need?

Purpose: I've just added a function that lets docker compose return only stdout, so that TLS Scripts followups can get just the Certificate ARN from stdout and clean up the resource during the test cleanup stage.

Copy link
Contributor

@yorinasub17 yorinasub17 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Had some comments for improving the implementation slightly.

The one other thing to add is a unit or integration test for this function. You should be able to reuse the test for RunDockerCompose for this purpose (https://github.com/gruntwork-io/terratest/blob/master/test/packer_docker_example_test.go)

modules/docker/docker_compose.go Outdated Show resolved Hide resolved
modules/docker/docker_compose.go Show resolved Hide resolved
@yorinasub17 yorinasub17 self-assigned this Oct 1, 2020
rhoboat and others added 2 commits October 1, 2020 14:22
Co-authored-by: Yoriyasu Yano <430092+yorinasub17@users.noreply.github.com>
@rhoboat
Copy link
Contributor Author

rhoboat commented Oct 1, 2020

@yorinasub17 This test is failing, added in this commit, because the stdout actually includes a lot more stuff than I expect. The stdout should only contain "stdout: message", but it contains all of this:

Error:      	"Attaching to testdockercomposestdoutexample_bash_script_1
        	bash_script_1  | stderr: error
        	bash_script_1  | stdout: message
        	testdockercomposestdoutexample_bash_script_1 exited with code 0
        	" should not contain "stderr: error"

modules/docker/docker_compose.go Outdated Show resolved Hide resolved
test/docker_stdout_example_test.go Show resolved Hide resolved
test/docker_stdout_example_test.go Outdated Show resolved Hide resolved
test/docker_stdout_example_test.go Outdated Show resolved Hide resolved
@yorinasub17
Copy link
Contributor

The stdout should only contain "stdout: message", but it contains all of this:

Ahhhh this might not be possible then... Frustratingly, I can't find the docs on where they output the logs to. Let me try a few things to see if I can empirically validate.

@yorinasub17
Copy link
Contributor

Ok yea both docker-compose and docker logs streams all the log output to STDOUT, including STDERR! This is especially frustrating considering that the logs are actually tracking the source (https://docs.docker.com/config/containers/logging/json-file/).

So it looks like this approach and function is not going to work 😞

@yorinasub17
Copy link
Contributor

yorinasub17 commented Oct 2, 2020

We probably need to add a function that extracts the stdout and stderr logs using the raw log files. I found the command that gives you the path to the raw log file which has the metadata on where it came from

docker inspect --format='{{.LogPath}}' DOCKER_IMAGE_ID

But... this is only accessible from the VM on mac osx so you can't actually see it directly and we'd need to yak shave a way to get into the VM...

Co-authored-by: Yevgeniy Brikman <brikis98@users.noreply.github.com>
@rhoboat
Copy link
Contributor Author

rhoboat commented Oct 2, 2020

@yorinasub17 However, when I source this change here locally in the TLS script, it's able to pull just the right info that's echoed to stdout, meaning the ARN. I was trying to figure out what the difference would be. Any ideas why it would work when used in service catalog but not here?

@rhoboat
Copy link
Contributor Author

rhoboat commented Oct 2, 2020

Ah, I solved the problem. If I call the bash_script with docker-compose run bash_script rather than up, it works.

@rhoboat
Copy link
Contributor Author

rhoboat commented Oct 2, 2020

🎉 Thanks @brikis98 and @yorinasub17! Ready for (final?) review.

Copy link
Contributor

@yorinasub17 yorinasub17 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updates LGTM (assuming build passes)! Found one more nit where we should expose another function, but we can punt that to another PR to avoid another test cycle.

out, err := runDockerComposeE(t, true, options, args...)
require.NoError(t, err)
return out
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just realized that we should probably expose a RunDockerComposeAndGetStdOutE function as well.

@rhoboat
Copy link
Contributor Author

rhoboat commented Oct 2, 2020

Thanks for the review! Failures are not related to this change, so merging.

@rhoboat rhoboat merged commit 0c883d7 into master Oct 2, 2020
@yorinasub17
Copy link
Contributor

Don't forget to cut a new release!

@rhoboat
Copy link
Contributor Author

rhoboat commented Oct 3, 2020

Okay! Thanks for that. I was waiting for feedback on this! Will go ahead now.

@rhoboat
Copy link
Contributor Author

rhoboat commented Oct 3, 2020

Ah, before I do, did you see my comment in Slack? I wanted to make sure we were safe to do it, i.e. passing tests on the main branch. There were a few small failures in this branch but I merged because they were unrelated to this change. That's a bit of risky logic, though, and not how I'd normally like to do things. So my idea was to wait for green on master before cutting a release.

brikis98 added a commit that referenced this pull request Oct 12, 2020
It looks like #665 added the ability to run `docker-compose` and get `stdout`. As part of this work, a new example in `examples/docker-compose-stdout-example` was added that had a `bash_script.sh` that was used for testing. All this works fine, but it looks like #665 _also_ added a reference to `bash_script.sh` to the `docker-compose.yml` in `examples/packer-docker-example`... But not `bash_script.sh` itself, so it's making tests fail.

This seems like a copy/paste error, or perhaps a leftover of some previous test, so I'm removing this mention.
helayoty pushed a commit to helayoty/terratest that referenced this pull request Jan 26, 2021
It looks like gruntwork-io#665 added the ability to run `docker-compose` and get `stdout`. As part of this work, a new example in `examples/docker-compose-stdout-example` was added that had a `bash_script.sh` that was used for testing. All this works fine, but it looks like gruntwork-io#665 _also_ added a reference to `bash_script.sh` to the `docker-compose.yml` in `examples/packer-docker-example`... But not `bash_script.sh` itself, so it's making tests fail.

This seems like a copy/paste error, or perhaps a leftover of some previous test, so I'm removing this mention.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants