-
Notifications
You must be signed in to change notification settings - Fork 36
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
Updated Dockerfile to resolve build issues due to vintage #131
base: master
Are you sure you want to change the base?
Conversation
Merge refactor --------- Co-authored-by: Richard Worwood <[email protected]>
* Update supported_versions.json to latest * Update supported_versions.json * Update supported_versions.json * Update supported_versions.json * Update Dockerfile * Update Dockerfile * Update Dockerfile * Update Dockerfile * Update Dockerfile * Update Dockerfile * Update Dockerfile * Update Dockerfil formatting and safety * Push update to Dockerfile * specify devian version * add gosu * remove gosu * switch to curl * specify azure-cli version * simplify structure * update versions * change setuptools version * switch to 58 * uses DOCKERHUB_USERNAME secret as * correct push name --------- Co-authored-by: Richard Worwood <[email protected]>
* Update to push using secrets * Update Dockerfile "Remove line" --------- Co-authored-by: Richard Worwood <[email protected]>
Bumps [plexsystems/container-structure-test-action](https://github.com/plexsystems/container-structure-test-action) from 0.2.0 to 0.3.0. - [Release notes](https://github.com/plexsystems/container-structure-test-action/releases) - [Commits](plexsystems/container-structure-test-action@v0.2.0...v0.3.0) --- updated-dependencies: - dependency-name: plexsystems/container-structure-test-action dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Bumps [actions/upload-artifact](https://github.com/actions/upload-artifact) from 2 to 3. - [Release notes](https://github.com/actions/upload-artifact/releases) - [Commits](actions/upload-artifact@v2...v3) --- updated-dependencies: - dependency-name: actions/upload-artifact dependency-type: direct:production update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Bumps [actions/download-artifact](https://github.com/actions/download-artifact) from 2 to 3. - [Release notes](https://github.com/actions/download-artifact/releases) - [Commits](actions/download-artifact@v2...v3) --- updated-dependencies: - dependency-name: actions/download-artifact dependency-type: direct:production update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
…#9) * Create supported-versions.yml * Create test-harness for testing supported versions workflow * Update build-test.yml * Update supported-versions.yml * Update test-harness.yml * Update test-harness.yml * commit test for supported-versions workflow * test for matrix content * add latest version workflow * move from includes/ directory and rename to includes_ * update to use include_latest-version.yml --------- Co-authored-by: Richard Worwood <[email protected]>
…s using artifact repository (#11) * Simplified build-test.yml to remove need to upload and download images to artifact repository
Hello @Jubblin 👋 Thanks a lot for the incredible work injected into this PR 🤩 As a matter of fact, I'm looking for maintainers for this project (and also for the AWS sibling) ! |
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.
What a nice work @Jubblin , thanks a lot 🙏
Many questions and comments after this review 🙂
I will keep an eye closely on this MR to improve the project ASAP 🚀
RUN apt-get update && \ | ||
apt-get install --no-install-recommends -y \ | ||
curl=7.74.0-1.3+deb11u7 \ | ||
ca-certificates=20210119 \ | ||
unzip=6.0-26+deb11u1 \ | ||
gnupg=2.2.27-2+deb11u2 && \ | ||
apt-get clean && \ | ||
rm -rf /var/lib/apt/lists/* |
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.
As this Dockerfile follow the multi-stages build pattern :
- Optimizing throw-away stages is not required, as additional image layers won't end up in the final runtime stage
- Keeping each instruction in a separated layer allow a more efficient use of build caching
So I would suggest to revert this changes 😉
RUN curl -Os https://releases.hashicorp.com/terraform/${TERRAFORM_VERSION}/terraform_${TERRAFORM_VERSION}_SHA256SUMS && \ | ||
curl -Os https://releases.hashicorp.com/terraform/${TERRAFORM_VERSION}/terraform_${TERRAFORM_VERSION}_linux_amd64.zip && \ | ||
curl -Os https://releases.hashicorp.com/terraform/${TERRAFORM_VERSION}/terraform_${TERRAFORM_VERSION}_SHA256SUMS.sig | ||
COPY hashicorp.asc hashicorp.asc | ||
RUN gpg --import hashicorp.asc | ||
RUN gpg --verify terraform_${TERRAFORM_VERSION}_SHA256SUMS.sig terraform_${TERRAFORM_VERSION}_SHA256SUMS | ||
RUN gpg --import hashicorp.asc && \ | ||
gpg --verify terraform_${TERRAFORM_VERSION}_SHA256SUMS.sig terraform_${TERRAFORM_VERSION}_SHA256SUMS | ||
SHELL ["/bin/bash", "-o", "pipefail", "-c"] | ||
RUN grep terraform_${TERRAFORM_VERSION}_linux_amd64.zip terraform_${TERRAFORM_VERSION}_SHA256SUMS | sha256sum -c - | ||
RUN unzip -j terraform_${TERRAFORM_VERSION}_linux_amd64.zip | ||
RUN grep terraform_${TERRAFORM_VERSION}_linux_amd64.zip terraform_${TERRAFORM_VERSION}_SHA256SUMS | sha256sum -c - && \ | ||
unzip -j terraform_${TERRAFORM_VERSION}_linux_amd64.zip |
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.
Same comment as above 🙂
RUN apt-get update && \ | ||
apt-get install -y --no-install-recommends \ | ||
python3=${PYTHON_MAJOR_VERSION}.2-3 \ | ||
python3-pip=20.3.4-4+deb11u1 && \ | ||
apt-get clean && \ | ||
rm -rf /var/lib/apt/lists/* && \ | ||
pip3 install --no-cache-dir setuptools==58 && \ | ||
pip3 install --no-cache-dir azure-cli==${AZURE_CLI_VERSION} |
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.
Same comment as above here too 🙂
[![lint-dockerfile](https://github.com/worwoods/terraform-azure-cli/actions/workflows/lint-dockerfile.yml/badge.svg)](https://github.com/worwoods/terraform-azure-cli/actions/workflows/lint-dockerfile.yml) | ||
[![build-test](https://github.com/worwoods/terraform-azure-cli/actions/workflows/build-test.yml/badge.svg)](https://github.com/worwoods/terraform-azure-cli/actions/workflows/build-test.yml) | ||
[![push-latest](https://github.com/worwoods/terraform-azure-cli/actions/workflows/push-latest.yml/badge.svg)](https://github.com/worwoods/terraform-azure-cli/actions/workflows/push-latest.yml) | ||
[![release](https://github.com/worwoods/terraform-azure-cli/actions/workflows/release.yml/badge.svg)](https://github.com/worwoods/terraform-azure-cli/actions/workflows/release.yml) | ||
|
||
[![Update Docker Hub Description](https://github.com/zenika-open-source/terraform-azure-cli/actions/workflows/dockerhub-description.yml/badge.svg)](https://github.com/zenika-open-source/terraform-azure-cli/actions/workflows/dockerhub-description.yml) | ||
[![Update Docker Hub Description](https://github.com/worwoods/terraform-azure-cli/actions/workflows/dockerhub-description.yml/badge.svg)](https://github.com/worwoods/terraform-azure-cli/actions/workflows/dockerhub-description.yml) | ||
[![License](https://img.shields.io/badge/License-Apache%202.0-blue.svg)](https://opensource.org/licenses/Apache-2.0) | ||
[![Docker Pulls](https://img.shields.io/docker/pulls/zenika/terraform-azure-cli.svg)](https://hub.docker.com/r/zenika/terraform-azure-cli/) | ||
[![Docker Pulls](https://img.shields.io/docker/pulls/worwoods/terraform-azure-cli.svg)](https://hub.docker.com/r/worwoods/terraform-azure-cli/) | ||
|
||
<p align="center"> | ||
<a href="https://azure.microsoft.com"><img width="200" src="https://github.com/Zenika/terraform-azure-cli/raw/master/resources/azure-logo.png"></a> | ||
<a href="https://www.terraform.io/"><img width="200" src="https://github.com/Zenika/terraform-azure-cli/raw/master/resources/terraform-logo.png"></a> | ||
<a href="https://azure.microsoft.com"><img width="200" src="https://github.com/worwoods/terraform-azure-cli/raw/master/resources/azure-logo.png"></a> | ||
<a href="https://www.terraform.io/"><img width="200" src="https://github.com/worwoods/terraform-azure-cli/raw/master/resources/terraform-logo.png"></a> | ||
</p> |
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 believe you changes this to test the badges and images, but we want to keep upstream references on https://github.com/zenika-open-source/terraform-azure-cli here 😉
Available image tags can be found on the Docker Hub registry: [zenika/terraform-azure-cli](https://hub.docker.com/r/zenika/terraform-azure-cli/tags) | ||
Available image tags can be found on the Docker Hub registry: [worwoods/terraform-azure-cli](https://hub.docker.com/r/worwoods/terraform-azure-cli/tags) |
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.
Same here, we want to keep the reference on Zenika registry
run: docker image build . --file Dockerfile --build-arg TERRAFORM_VERSION=${{ matrix.tf_version }} --build-arg AZURE_CLI_VERSION=${{ matrix.azcli_version }} --tag ${ORGANIZATION}/${IMAGE_NAME}:${IMAGE_RELEASE_TAG} | ||
run: docker image build . --file Dockerfile --build-arg TERRAFORM_VERSION=${{ matrix.tf_version }} --build-arg AZURE_CLI_VERSION=${{ matrix.azcli_version }} --tag ${{ secrets.DOCKERHUB_USERNAME }}/${IMAGE_NAME}:${IMAGE_RELEASE_TAG} |
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.
Same feedback as in push-latest workflow, we might be better off keeping ORGANIZATION env var
@@ -0,0 +1,50 @@ | |||
name: Load Supported Versions |
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.
Maybe I didn't understand correctly, but aren't include_latest-version
and include_supported-versions.yml
a bit of a duplication ?
The only difference I can sport is sort | reverse
VS sort | .[-1]
for both TF and AWSCLI versions, I'm not sure to quite understand the use case.
EDIT :
Oh okay the difference is the latest version for each VS a matrix of all versions for each tool.
In that cas, a plural on outputs would be nice 😉
matrix: | ||
description: "Matrix of supported versions" | ||
value: ${{ jobs.load_versions.outputs.matrix }} | ||
azcli_version: |
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.
azcli_version: | |
azcli_versions: |
Beware of references elsewhere in the code
azcli_version: | ||
description: "Matrix of supported azcli versions" | ||
value: ${{ jobs.load_versions.outputs.azcli_version }} | ||
tf_version: |
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.
tf_version: | |
tf_versions: |
Beware of references elsewhere in the code
@@ -0,0 +1,43 @@ | |||
name: Load Supported Versions |
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.
As you stated in the name, it might be better to keep the file name plural as latest version is exposed for multi tools, what do you think ?
That would be: .github/workflows/include_latest-versions.yml
Updated Dockerfile to resolve Dockerfile build issues, updated actions to the latest versions, and made workflows more generic so they can be built on more simply by others moving forward.