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

Docker #37

Merged
merged 57 commits into from
Aug 9, 2021
Merged

Docker #37

merged 57 commits into from
Aug 9, 2021

Conversation

qdm12
Copy link
Contributor

@qdm12 qdm12 commented Jul 31, 2021

Work in progress, almost ready!

  • Docker configuration
  • Documentation
  • CI to build and push to Docker Hub
    • Set docker hub password as a Github repository secret
    • Configure GA to build and push 2fauth/2fauth to Docker Hub
    • Configure Dockerfile and GA for cross building
    • Try to cross build natively in Docker build for faster builds without emulation

Also see #39

Copy link

@MohamedElashri MohamedElashri left a comment

Choose a reason for hiding this comment

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

That's great, I really liked the verify part but I still don't see the action doing native cross build yet. I understand that you might think that native cross build is better but buildx use BUILDKIT which is some sort of emulation (not through QEMU). What I usually understand from native build is that you build the images on desired arch and then use manifest to combine them but this might be overstratch now.

Anyway I see this GA is very good, we just need to test how fast it will be for each push. we might don't want to build for linux/arm/v6 as I think no one will host on it.

Last thing I still don't understand what you are trying to do by limiting build to amd64 if we want to push to dev branch. I think it is important to keep cross build for testing also because if @Bubka used something in the future that is not available for ARM then this will break out things for ARM versions.

@MohamedElashri
Copy link

@qdm12 I forgot to mention you 😅

@qdm12
Copy link
Contributor Author

qdm12 commented Aug 4, 2021

Don't worry I get notifications, I just haven't got the time to answer.

If you look at the Dockerfile, some of the steps are run only once on the native build platform to build for N target architectures. I think it's just composer dependencies setup, since other things (essentially apt install) are platform specific so must be emulated.

On my machine, building for 1 non native arch or 3 non native arch takes about the same time (its mostly network I guess), although it's much quicker to build for the native platform only (30sec vs 2min).

That's one of the reasons I limited branch building to amd64, since I don't think people would really pull a branch based image, although it can be useful to test on our machines (amd64). But we can change that, I don't mind.

@qdm12
Copy link
Contributor Author

qdm12 commented Aug 4, 2021

I changed the base image to be alpine:3.14 to have a tinier image (from 180MB to 84MB).

Cross building is now much quicker than with Debian (I guess apk > apt?). Without docker cache, it takes:

  • 4s to build for armv7
  • 5s to build armv6,armv6 and arm64 in parallel

Although it can take 10-30s to pull all the base images for each architecture, if these are not present/cached in the GA worker.

I also tried basing it on php:7.3-fpm-alpine3.14 which makes building even quicker (saving 1 or 2s only though), but the resulting image is 120MB, so I would suggest we don't do that.

qdm12 added 3 commits August 4, 2021 16:35
- CPU arch compatibility
- Assumption on path being `/yourpath`
- Fix chown from 33 to 1000
- Warning to backup database beforing updating
- Add tagged images information
- Update implementation details
- Remove TODOs as they are all done
@qdm12
Copy link
Contributor Author

qdm12 commented Aug 4, 2021

@MohamedElashri @Bubka I think this is ready now, I'm also curious to see how Github Actions performs (and hope it works too 😉).

Feel free to take your time to review it and to comment the code with questions if you have any. A few useful links:

@Bubka Bubka merged commit d1927fc into Bubka:master Aug 9, 2021
@qdm12
Copy link
Contributor Author

qdm12 commented Aug 10, 2021

@Bubka Not my repository, but you should squash and merge instead of merging all the commits next time 😉

You can set that as the default behavior in the repository settings.

Also there was a bug in my if block for the Docker hub description and publish workflows, can you try squash and merge #41 to see if it runs the Github actions correctly? I may have to do a few tiny one-liner PRs until this gets fixed, since I cannot really test it locally AFAIK.

@Bubka
Copy link
Owner

Bubka commented Aug 10, 2021

Yeah I forgot to squash, thanks for the heads-up regarding the default behavior.

@Bubka Bubka added the docker dockerfile or docker-compose setup label Sep 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docker dockerfile or docker-compose setup
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add a docker-compose setup for fast setup
3 participants