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

Create a common builder container #616

Merged
merged 10 commits into from
Jan 9, 2020
Merged

Create a common builder container #616

merged 10 commits into from
Jan 9, 2020

Conversation

jahkeup
Copy link
Member

@jahkeup jahkeup commented Dec 27, 2019

Issue #, if available:

#592 #541

Description of changes:

This change brings in a Dockerfile for a common base container - a build environment - for use in codebuild for most, if not all, stages that are needed at this time. Related environment improvements were wired in with the changes to the container's "entrypoint" (quoted because this is actually overridden and fully controlled by the CodeBuild service agent).

The use of a common image doesn't significantly reduce the build time from what I can see in my build history with this particular set of changes, but I think this may be due to the options provided to Docker (which causes it to use the less performant vfs backend). I'll check to see if using overlay2 in the CodeBuild task gets us some improvement there.

Until the CodeBuild projects and buildspecs are updated this image will not be used without further intervention. I've included a change to the thar-pr-build CodeBuild buildspec to show what these will look like. If folks are happy with the changes, I can tie in changes to the other buildspecs as well.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

tools/infra/Dockerfile Outdated Show resolved Hide resolved
Copy link
Contributor

@jhaynes jhaynes left a comment

Choose a reason for hiding this comment

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

I think moving the build container specific bits to their own directory, e.g. tools/infra/containers/ might help clarify what everything is for. Currently, it isn't obvious that the env/bin directory is for scripts to facilitate the containerized build.

@jahkeup
Copy link
Member Author

jahkeup commented Jan 1, 2020

I think moving the build container specific bits to their own directory, e.g. tools/infra/containers/ might help clarify what everything is for

I agree! I've moved things around in the latest commits along with an update to the README that's there.

I'll update the Dockerfile to add some context in comments, I'll circle back with this next year.

Copy link
Contributor

@zmrow zmrow left a comment

Choose a reason for hiding this comment

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

If we can add a few more comments and headers (mentioned below), then this looks good!

@jahkeup
Copy link
Member Author

jahkeup commented Jan 7, 2020

@zmrow added some comments and took the opportunity to expand some of the conditionals to help explain the flow in the changed scripts.

@jahkeup jahkeup marked this pull request as ready for review January 7, 2020 19:22
Copy link
Contributor

@zmrow zmrow left a comment

Choose a reason for hiding this comment

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

🖍

Additional tools are required to run the AMI building stage and
are common enough to merit being installed by default - eg: aws cli.
This uses the commands provided in the build environment instead of
performing the on-demand installation for building within.
@jahkeup
Copy link
Member Author

jahkeup commented Jan 9, 2020

Rebased onto latest develop and changing merge target to a "feature" branch to prepare in parts for switching to the containerized environments.

@jahkeup jahkeup changed the base branch from develop to ci-containers January 9, 2020 01:04
@jahkeup jahkeup merged commit 65bda1e into ci-containers Jan 9, 2020
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.

3 participants