Create a way to get the log from the docker container. #147
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Coverage report. |
tfoote
requested changes
Mar 8, 2018
superflore/docker.py
Outdated
| cmd_string = self.get_command(tmp) | ||
| if show_cmd: | ||
| msg = "Running container with command string '%s'..." | ||
| info(msg % self.get_command()) |
There was a problem hiding this comment.
You shouldn't regenerate the command here but use cmd_string
There was a problem hiding this comment.
The issue with that is the fact that get_command() without a log file will return the list, but, otherwise, shows a &&> /tmp/[tmp directory/log.txt file.
I could perhaps just do a .replace, but I don't think it's any more efficient at that point.
Or, if you want, I can just have it show the temporary log file mapping.
There was a problem hiding this comment.
I think it's better to show the real values. If you mention the mounting right after that would allow people to debug easily.
| @@ -69,3 +69,17 @@ def test_pull(self): | |||
| docker_instance.pull('allenh1', 'ros_gentoo_base') | |||
| docker_instance.add_bash_command("echo Hello, Gentoo") | |||
| docker_instance.run() | |||
|
|
|||
| def test_get_command(self): | |||
There was a problem hiding this comment.
It would be good to add a test of the contents of the log output too.
There was a problem hiding this comment.
Yep, that's a good idea. I'll get that going right now, then!
|
@tfoote That great JSON error has resurfaced in the last test I added (because of the fact it has to run the container). I'm giving that one more go before I remove it from the CI tests. |
|
tfoote
reviewed
Mar 12, 2018
superflore/docker.py
Outdated
| cmd_string = self.get_command(tmp) | ||
| if show_cmd: | ||
| msg = "Running container with command string '%s'..." | ||
| info(msg % self.get_command()) |
There was a problem hiding this comment.
I think it's better to show the real values. If you mention the mounting right after that would allow people to debug easily.
superflore/docker.py
Outdated
| ) | ||
| ok("Docker container exited.") | ||
| except docker.errors.ContainerError: | ||
| err("Docker container exited with errors.") |
There was a problem hiding this comment.
It might be good to state where the log file can be found for future reference here. In case someone wants to run it through grep or something.
There was a problem hiding this comment.
I was considering adding a --log-file flag. I think I'll go ahead and do that. As of now, the file gets removed (since it's in a tempfile).
allenh1
force-pushed
the
add-docker-output
branch
from
9eb04d8 to
1471d5e
Compare
allenh1
force-pushed
the
add-docker-output
branch
from
2798ea6 to
30189c5
Compare
|
Ok, that should just about do it for this one! You can now specify where to send the log. |
tfoote
approved these changes
Mar 13, 2018
zffgithub
pushed a commit
to zffgithub/superflore
that referenced
this pull request
Apr 11, 2023
…cture#147) * Added a `--verbose` flag * Added a `--log-file` argument to specify the name and path to store the log file.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This creates a logger mechanism for the docker container. We create a temporary directory using
TempfileManager, then send thestderrandstdoutstreams into the file in append mode.This allows us to grab the output even when commands return an error code, which is more desirable than just grabbing the output of successful commands (imo).
ToDo: parameterize the verbosity, add tests.
connects to #146