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: Full revamp of Dockerfile #319

Closed
wants to merge 14 commits into from
Closed

Conversation

balihb
Copy link
Contributor

@balihb balihb commented Jan 11, 2022

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

Rewritten and generalized the install steps in Dockerfile.

  • hadolint
  • dive
  • container-structure-test
  • install steps now easier to write (syntax highlight) and debug
  • install steps can be checked separately with ShellCheck
  • arm Docker image
  • some shellcheck fix

How has this code been tested

Lots of local Docker build :D

@balihb balihb changed the title full revamp of Dockerfile feat: full revamp of Dockerfile Jan 11, 2022
@balihb balihb changed the title feat: full revamp of Dockerfile feat: Full revamp of Dockerfile Jan 11, 2022
@MaxymVlasov
Copy link
Collaborator

Hi,
Thank you for your contribution.
I will check what of all these changes can be used in this repo.

@balihb
Copy link
Contributor Author

balihb commented Jan 11, 2022

any chance to get the script based install into the repo? it is producing a more reliable and smaller image

@MaxymVlasov
Copy link
Collaborator

MaxymVlasov commented Jan 11, 2022

Moved to #322

  • hadolint
  • some shellcheck fix (half of them, that works fine)

Very good findings. I think they should be added to #318

  • dive
  • container-structure-test

For me, it is more important to have a complete picture of the installation process in one place than:

  • install steps now easier to write (syntax highlight) and debug

Here no something complex to be need have shellcheck:

  • install steps can be checked separately with ShellCheck

Not needed:

  • arm Docker image
    Because all CI now on 95+% is x86_64, and arm Mac can in x86_64. Introduced changes, for me, looks too complicated, when this functional not needed or the need for it is so small that it can be ignored

@yermulnik
Copy link
Collaborator

I'm looking at the list of commits which is still receiving new commits and hence the questions: is this PR still WIP or is ready for review? In the former case would you mind converting it into Draft PR and switch to Ready for review once the work is completed? Thanks for your contribution!

@balihb
Copy link
Contributor Author

balihb commented Jan 11, 2022

I'm looking at the list of commits which is still receiving new commits and hence the questions: is this PR still WIP or is ready for review? In the former case would you mind converting it into Draft PR and switch to Ready for review once the work is completed? Thanks for your contribution!

it is ready. I just found a bug.

@MaxymVlasov
Copy link
Collaborator

The difference between #319 and #322 is 26MB or 4,5%. So yes, It's an improvement but things like docker-scripts/install-from-github.sh are too overwhelmed and negate this achievement

REPOSITORY                                  TAG                       IMAGE ID       CREATED              SIZE
pre-commit-terraform                        319                       732da69c5fb3   About a minute ago   548MB
pre-commit-terraform                        refactor_based_on_pr319   223377a61fbf   2 hours ago          574MB

@balihb
Copy link
Contributor Author

balihb commented Jan 11, 2022

IMHO having so many multiline shellscript in the dockerfile just makes it harder to see what is happening. It is easy to understand a single well formatted shellscript in itself

For me, it is more important to have a complete picture of the installation process in one place than:

* install steps now easier to write (syntax highlight) and debug

Most home clusters are using ARM. also in some clouds the ARM nodes are cheaper. The extra complexity is just one shellscript and two env var.

Not needed:

* arm Docker image
  Because all CI now on 95+% is x86_64, and arm Mac can in x86_64. Introduced changes, for me, looks too complicated, when this functional not needed or the need for it is so small that it can be ignored

@balihb
Copy link
Contributor Author

balihb commented Jan 11, 2022

The difference between #319 and #322 is 26MB or 4,5%. So yes, It's an improvement but things like docker-scripts/install-from-github.sh are too overwhelmed and negate this achievement

REPOSITORY                                  TAG                       IMAGE ID       CREATED              SIZE
pre-commit-terraform                        319                       732da69c5fb3   About a minute ago   548MB
pre-commit-terraform                        refactor_based_on_pr319   223377a61fbf   2 hours ago          574MB

not my repo, so up to you...

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