-
Notifications
You must be signed in to change notification settings - Fork 594
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
Update Go dependency version to 1.12 and update docker sandboxes #432
Conversation
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: Docker file for gotip needs to set destination to v2 sdk for ADD Docker operation. Also, not sure if docker can pull aws-golang:tip;
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, same comments to update docker tag to aws-sdk-go-v2
and Dockerfile.test.gotip
SDK's build path should be aws-sdk-go-v2
Fixed the tags for the the v2 SDK containers. I also fixed the more generic aws-golang:tip that has been broken in both SDK versions due to Debian Wheezy repositories no longer being available. I've upgraded this to use the Debian buster repos and will port the same change for the gotip docker file to v1. |
@@ -0,0 +1,13 @@ | |||
FROM ubuntu:18.04 |
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.
Hey, I just stumbled across this PR and was curious to take a look. The two consecutive FROM
lines don't work the way I think you think they work; the golang:1.13
image is not layered on top of the ubuntu:18.04
image but rather replaces it. (Multiple FROM
lines are useful in multi-stage builds where artifacts are constructed in one container and copied to another.) If you run the image that results from this Dockerfile, you'll see that it's based on Debian Buster instead of Ubuntu.
$ docker run --rm internal/aws-sdk-go-v2:1.13 cat /etc/os-release
PRETTY_NAME="Debian GNU/Linux 10 (buster)"
NAME="Debian GNU/Linux"
VERSION_ID="10"
VERSION="10 (buster)"
VERSION_CODENAME=buster
ID=debian
HOME_URL="https://www.debian.org/"
SUPPORT_URL="https://www.debian.org/support"
BUG_REPORT_URL="https://bugs.debian.org/"
Compare to:
$ docker run --rm ubuntu:18.04 cat /etc/os-release
NAME="Ubuntu"
VERSION="18.04.1 LTS (Bionic Beaver)"
ID=ubuntu
ID_LIKE=debian
PRETTY_NAME="Ubuntu 18.04.1 LTS"
VERSION_ID="18.04"
HOME_URL="https://www.ubuntu.com/"
SUPPORT_URL="https://help.ubuntu.com/"
BUG_REPORT_URL="https://bugs.launchpad.net/ubuntu/"
PRIVACY_POLICY_URL="https://www.ubuntu.com/legal/terms-and-policies/privacy-policy"
VERSION_CODENAME=bionic
UBUNTU_CODENAME=bionic
If you want to test compiling and run your unit tests on Ubuntu, you'd need to use the Ubuntu base image and install the Go compiler into it.
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.
Thanks for pointing this out! We'll fixup the Dockerfiles to use the correct syntax. I don't think we explicitly need testing on specific Os versions in these tests. At least not yet. The base Debian images should be sufficient.
Updates Docker sandbox images for testing go1.12 and go1.13 which are the currently supported language releases. Additionally updates the SDK's module dependency on go1.12.