-
Notifications
You must be signed in to change notification settings - Fork 20
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 script
binary and screen
to container
#27
Conversation
`script` is a script utility in the `util-linux` package that the SSM agent uses for session logging when enabled. `screen` is also installed to avoid log data from being truncated.
# Validate script binary | ||
RUN /usr/bin/script &>/dev/null |
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.
Nit: this adds an extra layer, should we instead test the container with actual testing instead of using the Dockerfile to validate the binary?
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.
We certainly could, but I was worried that it would be "out of sight, out of mind" by anyone building the Dockerfile directly. What do folks think?
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 remember asking for this and hadn't considered the extra layer. I still kinda like the check here as a belt-and-suspenders method for ensuring things work. It might be nice to add a bit more to the comment explaining that we're doing this check on account of copying a dynamically linked binary.
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 aside from outstanding comments.
Issue number:
#24
Description of changes:
script
is a script utility in theutil-linux
package that the SSMagent uses for session logging when enabled.
screen
is also installedto avoid log data from being truncated (per Logging session activity).
Testing done:
aws-ecs-1
ami with new control container set in userdata.sudo sheltie
.Additional testing done:
Confirmed that session logging to S3 bucket works. (thanks, @etungsten!)
Terms of contribution:
By submitting this pull request, I agree that this contribution is dual-licensed under the terms of both the Apache License, version 2.0, and the MIT license.