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

Initial docker image creation workflows. #9368

Merged
merged 1 commit into from
Jul 15, 2020

Conversation

aarlt
Copy link
Member

@aarlt aarlt commented Jul 9, 2020

Closes #9213. Replaces #9254.

scripts/docker/buildpack-deps/README.md

buildpack-deps docker images

The buildpack-deps docker images are used to compile and test solidity within our CI.

GitHub Workflow

The creation of the images are triggered by a single workflow, defined in .github/workflows/buildpack-deps.yml. For each resulting buildpack-deps docker image a strategy is defined in the workflow file - the image variant. The workflow gets triggered, if any Dockerfile defined in scripts/docker/buildpack-deps/Dockerfile.* were changed within the PR.

Versioning

The version of the docker images can be defined within the Dockerfile with LABEL version. A new docker image will only be created and pushed, if the new version is incremented by 1 compared with the version of the Dockerfile located in develop.

Build, Test & Push

Note that the whole workflow - including all defined strategies (image variants) - will be triggered, even if only a single Dockerfile was change. The full workflow will only gets executed, if the corresponding Dockerfile was changed. The execution of workflows of unchanged Dockerfiles will not continue and just return success. See scripts/ci/docker_upgrade.sh.

If the version check was successful, the docker image will be built using the Dockerfile located in scripts/docker/buildpack-deps/Dockerfile.*.

The resulting docker image will be tested by executing the corresponding scripts/ci/buildpack-deps_test_* scripts. These scripts are normally symlinked to scripts/ci/build.sh, except for the buildpack-deps ubuntu1604.clang.ossfuzz docker image, that is symlinked to scripts/ci/build_ossfuzz.sh. These scripts scripts/ci/build.sh and scripts/ci/build_ossfuzz.sh are also used by CircleCI, see .circleci/config.yml.

If the tests passed successfully, the docker image will get tagged by the version defined within the corresponding Dockerfile. Finally, a comment will be added to the PR that contains the full repository, version and repository digest of the freshly created docker image.

@aarlt aarlt requested a review from ekpyron July 9, 2020 03:20
Copy link
Member

@ekpyron ekpyron left a comment

Choose a reason for hiding this comment

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

This definitely goes in the right direction, but I think this still needs quite some work to get really nice, clean and simple!

scripts/docker/buildpack/Dockerfile.ubuntu2004 Outdated Show resolved Hide resolved
cmake/jsoncpp.cmake Outdated Show resolved Hide resolved
.github/actions/pr-comment/package.json Outdated Show resolved Hide resolved
.github/workflows/buildpack/check-version.sh Outdated Show resolved Hide resolved
.github/workflows/buildpack/check-version.sh Outdated Show resolved Hide resolved
.github/workflows/buildpack-ubuntu2004.yml Outdated Show resolved Hide resolved
.github/workflows/buildpack-ubuntu2004.yml Outdated Show resolved Hide resolved
.github/workflows/buildpack/test-image.sh Outdated Show resolved Hide resolved
@aarlt aarlt force-pushed the docker-workflow branch from 4dc11b0 to 3b73ae8 Compare July 10, 2020 02:53
@aarlt aarlt force-pushed the docker-workflow branch 2 times, most recently from 636941e to 5df8715 Compare July 10, 2020 20:50
Copy link
Member

@ekpyron ekpyron left a comment

Choose a reason for hiding this comment

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

Yep, now this is really starting to look nice and simple :-)!

scripts/build.sh Outdated Show resolved Hide resolved
scripts/ci/build.sh Show resolved Hide resolved
scripts/docker/buildpack/Dockerfile.emscripten Outdated Show resolved Hide resolved
@aarlt aarlt force-pushed the docker-workflow branch from bc1ea0b to 2f86d21 Compare July 12, 2020 01:34
@ekpyron
Copy link
Member

ekpyron commented Jul 13, 2020

We should probably document the process at least in some README or in some other appropriate place by the way.
EDIT: maybe not in the actual documentation, though, but rather near the actual Dockerfile's, since it cannot be done by external contributors anyways, so documenting it would mainly be for future reference for us.

@aarlt aarlt force-pushed the docker-workflow branch 3 times, most recently from 028791b to a9a06f3 Compare July 13, 2020 13:04
@aarlt
Copy link
Member Author

aarlt commented Jul 13, 2020

Once this is ok, I will increment the versions to create the first official docker images.

@ekpyron
Copy link
Member

ekpyron commented Jul 13, 2020

So just to be clear: the actions failing now is by design and intentional - they'll start working on the next PR that increments the version - and @aarlt has a fork in which that was already demonstrated do work (haven't you ;-))?

@aarlt
Copy link
Member Author

aarlt commented Jul 13, 2020

@ekpyron yes sure :) See: aarlt#22 .. the failure shown there right now is there because I set the version back to version zero.. https://github.com/aarlt/solidity/pull/22/commits

ekpyron
ekpyron previously approved these changes Jul 13, 2020
Copy link
Member

@ekpyron ekpyron left a comment

Choose a reason for hiding this comment

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

Ok, so at the time of writing aarlt#23 almost went through demonstrating how this will work in the end.
It shows what happens once we merge this and update the versions labels in the dockerfiles to 1 - it will build test and push each one and post it's SHA256 hash in the PR for us to use in CircleCI.

I'm approving this now, but I'd like someone to have a second eye on it to confirm that we can work with this now? (@chriseth, @cameel)

EDIT: As explaination of the Actions in this PR failing: they only work when the VERSION label in the dockerfiles are increased by one - we intentionally have them abort here, where there's no previous dockerfile with such a label in the repo (we create them with VERSION="0" as part of the PR).

ekpyron
ekpyron previously approved these changes Jul 13, 2020
Copy link
Member

@ekpyron ekpyron left a comment

Choose a reason for hiding this comment

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

[Reapproving after rebase - the comment above still holds]


if [[ $((PREV_VERSION + 1)) != $((NEXT_VERSION)) ]]; then
echo ""
echo "ERROR: Version label in Dockerfile was not incremented. You may need to change 'LABEL version' in '${DOCKERFILE}'. Aborting."
Copy link
Member

Choose a reason for hiding this comment

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

The current message is technically correct when taken literally but if I saw it, it would make me think that version was unchanged, not just different from expectation. Maybe it could be reworded a bit? Or maybe there should be a separate one if version was just not changed at all?

scripts/docker/buildpack-deps/README.md Outdated Show resolved Hide resolved
scripts/ci/docker_upgrade.sh Outdated Show resolved Hide resolved
scripts/ci/docker_upgrade.sh Outdated Show resolved Hide resolved
scripts/ci/docker_upgrade.sh Outdated Show resolved Hide resolved
scripts/ci/docker_upgrade.sh Outdated Show resolved Hide resolved
@aarlt aarlt force-pushed the docker-workflow branch 3 times, most recently from 77e80d0 to b55d2b8 Compare July 14, 2020 04:01
@ekpyron
Copy link
Member

ekpyron commented Jul 14, 2020

I really think one workflow per image with a proper trigger is nicer than unifying them with the strategy matrix and will avoid a lot of confusion down the road, so I think that's worth the duplication and I'd suggest to change that back.
Intentionally provoking failed runs on PRs doesn't seem like a good idea to me.

We could also have the update script in the action only run, if the corresponding docker file has any changes, but that will result in succeeding action runs on the PR for unchanged docker images - that's equally confusing. I expect github actions will - at some point in the future - provide some nicer means to avoid duplication like that (I saw some issue in that direction), but for now I would just keep it at separate actions.

scripts/ci/build.sh Outdated Show resolved Hide resolved
@aarlt aarlt force-pushed the docker-workflow branch from b55d2b8 to 5941a1f Compare July 14, 2020 16:48
@aarlt aarlt force-pushed the docker-workflow branch 5 times, most recently from 05aaff2 to 05ee728 Compare July 14, 2020 17:37
Copy link
Member

@ekpyron ekpyron left a comment

Choose a reason for hiding this comment

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

Ok, I'll once again approve it, but do you have another test run of this on your fork after all these changes in the end :-)?
Because if those pass now, I'd really say we merge this and postpone any further changes and cleanups for subsequent PRs.

ekpyron
ekpyron previously approved these changes Jul 14, 2020
@ekpyron
Copy link
Member

ekpyron commented Jul 14, 2020

There's trailing whitespace in the README though, bothering the style check script ;-).

cameel
cameel previously approved these changes Jul 14, 2020
Copy link
Member

@cameel cameel left a comment

Choose a reason for hiding this comment

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

I have nothing to add and all the stuff that could be imporoved is supposed to be handled in separate PRs so I'm marking this one as approved.

@aarlt aarlt dismissed stale reviews from cameel and ekpyron via e4f1f3e July 14, 2020 23:33
@aarlt aarlt force-pushed the docker-workflow branch from 05ee728 to e4f1f3e Compare July 14, 2020 23:33
@aarlt aarlt force-pushed the docker-workflow branch from e4f1f3e to aed424f Compare July 15, 2020 00:01
@aarlt
Copy link
Member Author

aarlt commented Jul 15, 2020

I did some final tests in aarlt#29 and aarlt#30 and it looks quite good. Should we first merge this and then increment the version to build the first images?

@ekpyron ekpyron merged commit 8226f3e into ethereum:develop Jul 15, 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.

[docker] Automated build of docker images.
4 participants