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

feat: Alpine based Docker image #239

Closed
wants to merge 26 commits into from
Closed

Conversation

balihb
Copy link
Contributor

@balihb balihb commented Oct 4, 2021

Put an x into the box if that apply:

  • This PR introduces breaking change.
  • This PR fixes a bug.
  • This PR adds new functionality.
  • This PR enhances existing functionality.

Description of your changes

The current debian/ubuntu mixed image is just not working. I've made an alpine based one.

How has this code been tested

In gitlab ci for some reason the commented out version file part of the Dockerfile is giving me 255, but it works fine locally with the same parameters. Nevertheless, this image works, the other is not.

@antonbabenko
Copy link
Owner

I am really not sure that we want to maintain extra platforms. If there is something not working with the one we already have, let's fix it.

@balihb
Copy link
Contributor Author

balihb commented Oct 4, 2021

I am really not sure that we want to maintain extra platforms. If there is something not working with the one we already have, let's fix it.

I'm fine with removing the old debian based image.

@antonbabenko
Copy link
Owner

I don't think we need Dockerfile.alpine :) The rest of the changes in this PR are reasonable and have to be evaluated/reviewed.

@balihb
Copy link
Contributor Author

balihb commented Oct 4, 2021

I hope this is better!

Copy link
Collaborator

@MaxymVlasov MaxymVlasov left a comment

Choose a reason for hiding this comment

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

Hi
Thank you for moving that to a more tiny image

But before I will test that it works as expected, please do the next things:

@@ -0,0 +1,2 @@
ignored:
- DL3059
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please, move this one to .pre-commit-config.yaml


# hadolint ignore=DL3018
RUN apk add --no-cache jq curl unzip && rm -rf "/var/cache/apk/*"
Copy link
Collaborator

Choose a reason for hiding this comment

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

--no-cache == rm -rf "/var/cache/apk/*"

Suggested change
RUN apk add --no-cache jq curl unzip && rm -rf "/var/cache/apk/*"
RUN apk add --no-cache jq curl unzip


# install build tools
# hadolint ignore=DL3013
RUN python3 -m pip install --no-cache-dir --upgrade pip
Copy link
Collaborator

Choose a reason for hiding this comment

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

You use python as a base image. Why do you think that it can include outdated pip?

I pretty sure that this step can be removed

Comment on lines +40 to +41
TERRAFORM_VERSION=$(curl -sS https://api.github.com/repos/hashicorp/terraform/releases/latest | jq -r .name | sed -r 's/v([0-9]+\.[0-9]\.+[0-9]+)/\1/g') && \
export TERRAFORM_VERSION \
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
TERRAFORM_VERSION=$(curl -sS https://api.github.com/repos/hashicorp/terraform/releases/latest | jq -r .name | sed -r 's/v([0-9]+\.[0-9]\.+[0-9]+)/\1/g') && \
export TERRAFORM_VERSION \
TERRAFORM_VERSION=$(curl -sS https://api.github.com/repos/hashicorp/terraform/releases/latest | jq -r .name | sed -r 's/v([0-9]+\.[0-9]\.+[0-9]+)/\1/g') \

Comment on lines +103 to +113
# In one environemt it gives me 255
#RUN F=tools_versions_info && \
# pre-commit --version >> $F && \
# terraform --version | head -n 1 >> $F && \
# (if [ "$CHECKOV_VERSION" != "false" ] || [ "${INSTALL_ALL}" = "true" ]; then echo "checkov $(checkov --version)" >> $F; else echo "checkov SKIPPED" >> $F ; fi) && \
# (if [ "$TERRAFORM_DOCS_VERSION" != "false" ] || [ "${INSTALL_ALL}" = "true" ]; then ./terraform-docs --version >> $F; else echo "terraform-docs SKIPPED" >> $F; fi) && \
# (if [ "$TERRAGRUNT_VERSION" != "false" ] || [ "${INSTALL_ALL}" = "true" ]; then ./terragrunt --version >> $F; else echo "terragrunt SKIPPED" >> $F ; fi) && \
# (if [ "$TERRASCAN_VERSION" != "false" ] || [ "${INSTALL_ALL}" = "true" ]; then echo "terrascan $(./terrascan version)" >> $F; else echo "terrascan SKIPPED" >> $F ; fi) && \
# (if [ "$TFLINT_VERSION" != "false" ] || [ "${INSTALL_ALL}" = "true" ]; then ./tflint --version >> $F; else echo "tflint SKIPPED" >> $F ; fi) && \
# (if [ "$TFSEC_VERSION" != "false" ] || [ "${INSTALL_ALL}" = "true" ]; then echo "tfsec $(./tfsec --version)" >> $F; else echo "tfsec SKIPPED" >> $F ; fi) && \
# cat $F
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is needed for debugging purposes.

https://github.com/antonbabenko/pre-commit-terraform/blob/master/.github/ISSUE_TEMPLATE/bug_report_docker.md?plain=1#L66-L67

If you have any idea how it can be realized in a different way - please, share

@MaxymVlasov MaxymVlasov self-assigned this Oct 4, 2021
@MaxymVlasov MaxymVlasov added area/docker feature New feature or request labels Oct 4, 2021
@MaxymVlasov MaxymVlasov changed the title Alpine based Docker image feat: Alpine based Docker image Oct 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/docker feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants