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: Unify Dockerfiles #364

Merged
merged 6 commits into from
Feb 12, 2024
Merged

feat: Unify Dockerfiles #364

merged 6 commits into from
Feb 12, 2024

Conversation

USA-RedDragon
Copy link
Contributor

@USA-RedDragon USA-RedDragon commented Feb 11, 2024

Context

  • Unify the Docker image for both amd64 and arm64 architectures so that arm64 users no longer need to use a separate tag. With this solution, no users will have to specify -arm64 tags since Docker will resolve the appropriate architecture image automatically.

Choices

There's no real magic to this unification, it uses:

As @sonroyaalmerol pointed out in #319 (comment), this could possibly require building future dependencies from source if added, though the arm64 emulation in GitHub Actions is likely to always be the slowest point in the image build. Using buildx now means that some of the workflow can run in parallel vs the default builder which was sequential, meaning it's likely the time to build has been reduced by a bit.

Test instructions

  1. Built for both architectures with docker buildx build --platform=linux/amd64,linux/arm64 . to verify build sanity
  2. On my amd64 desktop, docker buildx build --platform=linux/amd64 . --load -t palworld:test
  3. (still on amd64 desktop) docker run -p 8211:8211/udp --rm -it palworld:test and connected via Palworld to verify the server still runs
  4. On a Macbook Air M2, I disabled Rosetta in the Docker for Mac settings
  5. (still on Macbook) docker buildx build --platform=linux/arm64 . --load -t palworld:test
  6. (still on Macbook) docker run -p 8211:8211/udp --rm -it palworld:test and connected via Palworld to verify the server still runs

Checklist before requesting a review

  • I have performed a self-review of my code
  • I've added documentation about this change to the README.
  • I've not introduced breaking changes.

I have introduced breaking changes in that the -arm64 tags suffixes will no longer be required.

> Avoid use of wget without progress bar. Use `wget --progress=dot:giga <url>`. Or consider using `-q` or `-nv` (shorthands for `--quiet` or `--no-verbose`)
> Do not use --platform flag with FROM

While this would be a good idea for single-arch images, on amd64 this image must still resolve to avoid build errors but is not used in the final image
> Always tag the version of an image explicitly

Hadolint is unaware that the final FROM image is based on one of the two FROM's above, both of which are tagged. Therefore this is a false positive.
@USA-RedDragon
Copy link
Contributor Author

USA-RedDragon commented Feb 11, 2024

Hadolint makes two good points that I don't want to ignore:

Dockerfile:18 DL3007 warning: Using latest is prone to errors if the image will ever update. Pin the version explicitly to a release tag

This is the sonroyaalmerol/steamcmd-arm64:latest base image. I personally solved for this by pinning the sha256 digest in my fork (here), but I also utilize Renovate to get PRs when this changes (see the dependency dashboard under dockerfile here: USA-RedDragon/palworld-server-docker#3)

Dockerfile:41 DL3002 warning: Last USER should not be root

Which is solid advice in general. I would consider creating and using appropriate users to be out of scope for this PR, but I'd be happy to make a separate PR for this if y'all want it!

Dockerfile Outdated
FROM cm2network/steamcmd:root as base-amd64
# Ignoring --platform=arm64 as this is required for the multi-arch build to continue to work on amd64 hosts
# hadolint ignore=DL3029
FROM --platform=arm64 sonroyaalmerol/steamcmd-arm64:latest as base-arm64
Copy link
Contributor

Choose a reason for hiding this comment

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

One thing you could do is use sonroyaalmerol/steamcmd-arm64:root instead of the latest tag. This would also allow you to remove USER root from the other linting error. While this is definitely not a fix, at least you'll have the linting errors suppressed for now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh hey, that's you!

What's the difference between the root and latest tags? I noticed that the arm64 build used latest, so I was a bit leery of just swapping tags without understanding the difference, lest I unintentionally break something.

Though that would avoid the lint error, the "spirit" of the DL3007 lint rule would still be violated, as it's point is to avoid tags that aren't deterministic

Copy link
Contributor

Choose a reason for hiding this comment

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

The image is identical to cm2network/steamcmd (the base image used for amd64) but with the addition of Box64 and Box86 which adds compatibility for arm64. The root tag is literally just the latest tag but with root as its active user. The latest tag has steam as its active user as the steamcmd executable within the images are owned and can only be executed by steam.

The only reason the Dockerfile.arm64 used the latest tag was that I hadn't implemented the root tag yet when I PR-ed the arm64 support.

Well, I can definitely add some deterministic tags for version pinning in my image but I can't say the same for the original cm2network/steamcmd image.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for clarifying!

@win5923
Copy link
Contributor

win5923 commented Feb 11, 2024

Excellent!

@thijsvanloef thijsvanloef merged commit 7562f88 into thijsvanloef:main Feb 12, 2024
8 checks passed
@USA-RedDragon USA-RedDragon deleted the unify-dockerfile branch February 12, 2024 10:17
@celaraze celaraze mentioned this pull request Feb 22, 2024
3 tasks
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.

None yet

4 participants