-
Notifications
You must be signed in to change notification settings - Fork 185
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
refactor: Dockerfile and CI #69
base: main
Are you sure you want to change the base?
Conversation
- Build image from scratch - Copy terraform from its image - Use `-s -w` ldflags with trimpath to build rover with least size (14M) - Use UPX to compress the binray without any performance penalty that make binary size (4.3M) - Change rover path as terraform's - Create `/tmp` dir as its needed by rover - Copy certs for terraform
Thanks for submitting this PR! I have spotty internet for this week and don't have access to my laptop, but I'll review and merge once I get the chance Wanted to express how much I appreciate your work and I'm not just ignoring it 😄 |
@im2nguyen Do you have any plans for multi platform container images? I can contribute in that |
Hey @pratikbalar, what do you mean by multiplatform container images? |
Okay i don't know if you know but as multiple architecture binaries we are generating in release, we can also create multiple architecture container images You can see golang docker hub image E.g. let say if we pull More here |
Oh that's neat! Multi-arch builds would be a neat addition! |
See, I'm using |
Do you want to try ? In your local ? |
Hello! sorry i got back to you so late, can you provide instructions on how I should test locally? |
I can see some changes in |
@im2nguyen Do you think that image generation feature worth |
That's a good question. I think we can make it optional? One with image gen (chromium), the other one with it? What do you think @pratikbalar? |
Umm, I'm not with it. Because that will start to make confusion, and then we'll add slim variant, Also https://pkgs.alpinelinux.org/packages?name=chromium&branch=edge |
@im2nguyen Well I talked to my friends, They haven't used these but based on their knowledge they suggested me ⬇️ https://github.com/chromedp/chromedp let me know if you want me to involve more Plus, one of my friend also suggested running |
It's already using chromedp to access Chrome headlessly. The problem is that it still needs access Chrome to exists in the environment. This looks like a promising solution. https://hub.docker.com/r/chromedp/headless-shell/ Could we use this as a base and move the Terraform and rover binary? |
Always appreciate contributions 😄 Just let me know how I should test your changes. If it benefits the product for everyone (it does in this case), I'll be more than happy to merge |
Can you give me little time for this 😄
I haven't merge chrome changes, that's why. Let me push current changes. |
Also, do you/we want to maintain |
|
Nice! Is there a way for me to test this locally? |
Well, I don't know if you noticed, but I've edited |
@im2nguyen should I separate all the changes in different PR ? And have you tried, |
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.
Hello! Sorry, I was finally able to get to it. While testing it, I had trouble running the docker buildx bake
command, can you help me through it?
Also, I included notes about moving the Docker build instruction to its own dedicated page!
## Create local image | ||
docker buildx bake |
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 received the following error while building. Can you look into it? Thank you!
$ docker buildx bake
[+] Building 0.0s (0/0)
docker-bake.hcl:27
--------------------
25 | variable "TAGS" {
26 | default = [
27 | >>> "${REPO}:latest",
28 | "${REPO}:${VERSION}",
29 | "${REPO}:${VERSION}-${GIT_SHA}"
--------------------
error: docker-bake.hcl:27,8-12: Variables not allowed; Variables may not be used here., and 14 other diagnostic(s)
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.
these are my current docker specs. might be good to include this as prereqs for folks that are interested in building this locally
$ docker version
Client:
Cloud integration: 1.0.14
Version: 20.10.6
API version: 1.41
Go version: go1.16.3
Git commit: 370c289
Built: Fri Apr 9 22:46:57 2021
OS/Arch: darwin/amd64
Context: default
Experimental: true
Server: Docker Engine - Community
Engine:
Version: 20.10.6
API version: 1.41 (minimum version 1.12)
Go version: go1.13.15
Git commit: 8728dd2
Built: Fri Apr 9 22:44:56 2021
OS/Arch: linux/amd64
Experimental: false
containerd:
Version: 1.4.4
GitCommit: 05f951a3781f4f2c1911b05e61c160e9c30eaa8e
runc:
Version: 1.0.0-rc93
GitCommit: 12644e614e25b05da6fd08a38ffa0cfe1903fdec
docker-init:
Version: 0.19.0
GitCommit: de40ad0
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.
Can you docker buildx version
?
Mine is,
github.com/docker/buildx v0.7.1-docker 05846896d149da05f3d6fd1e7770da187b52a247
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 think this is due to docker version. Mine is,
$ docker version
Client:
Version: 20.10.12
API version: 1.41
Go version: go1.17.5
Git commit: e91ed5707e
Built: Mon Dec 13 22:31:40 2021
OS/Arch: linux/amd64
Context: builders
Experimental: true
Server:
Engine:
Version: 20.10.12
API version: 1.41 (minimum version 1.12)
Go version: go1.17.5
Git commit: 459d0dfbbb
Built: Mon Dec 13 22:30:43 2021
OS/Arch: linux/amd64
Experimental: false
containerd:
Version: v1.5.8
GitCommit: 1e5ef943eb76627a6d3b6de8cd1ef6537f393a71.m
runc:
Version: 1.0.3
GitCommit: v1.0.3-0-gf46b6ba2
docker-init:
Version: 0.19.0
GitCommit: de40ad0
Can you try upgrading your docker version if that won't break any of your existing projects.
Co-authored-by: Tu Nguyen <[email protected]>
Co-authored-by: Tu Nguyen <[email protected]>
Co-authored-by: Tu Nguyen <[email protected]>
And I don't know why, but now I'm getting this error on
|
Hey @pratikbalar, that was due to recent changes that included optional chaining. Did you pull + merge from the Line 3 in 2eaa45b
|
@im2nguyen Yeah, I've pull + merged main. Still getting same error. For this kind of situations, I've created a |
Was it able to resolve it? |
Still getting this same error on |
This is great! Thank you @im2nguyen @pratikbin. Looking forward to seeing this PR come live and building Docker images using GitHub Actions in a regular cadence. Happy to help with the testing. |
Overview
docker-bake
andbuildx
for building parallel multi stage docker image with better caching support withgha
-s -w
ldflags and trimpath more here/tmp
dir as its necessaryfor rovernpm build
as one testing workflowgo build = 24MB
-rwxr-xr-x 1 pratik pratik 17596884 Dec 18 19:28 rover
go build -trimpath -ldflags "-s -w" = 19M
-rwxr-xr-x 1 pratik pratik 19542016 Jan 14 19:43 rover
go build -trimpath -ldflags "-s -w" + UPX = 4.8M
-rwxr-xr-x 1 pratik pratik 4999224 Jan 14 19:43 rover
Terraform + UPX = ~13 MB from
Terraform + UPX = 60M to 13MB (part of slim container image)
May the source be with you