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

refactor(Apps/Docker): Remove Deno as a dependency from Docker Build #16898

Closed

Conversation

michaeldelago
Copy link
Contributor

@michaeldelago michaeldelago commented Aug 5, 2023

I can be contacted on discord as @mynameismeat. I accept DM's, though I strongly prefer a message in one of the channels. #support-general is probably fine (we can open up a thread), but I defer to Discord Admins to proper topic placement.

Changes Proposed:

  • Remove deno Dependency from Docker build
    • In the context of docker builds, Deno is called from shell, and then it generates shell commands to run. See apps/docker/docker-cmd.ts
    • Deno does not have a build for ARM64 Linux or Windows, meaning that the acore.sh script can only be run on MacOS (Apple Silicon)
  • ./acore.sh docker client-data has been moved and replaced with a service which downloads the client data in docker-compose.yml
  • Minor cleanup in the Dockerfile
    • blocks of multiple RUN commands of the same command (such as mkdir and chown) have been moved into one RUN
    • use of sudo has been wholly removed

Issues Addressed:

  • Resolves chore(bash/docker): Docker installation support for ARM64 machines #16882 without using an unofficial/unsupported build of Deno
    • In this PR I suggest that we could vendor the deno install script (before I edited my comment that we probably shouldn't be using an unofficial Deno build)
    • Vendoring further dependencies is probably a bad thing
      • We have to maintain/support it
      • It adds to the noise/bloat of the repo
  • sudo usage has been reduced
    • it's a relatively common issue noted in discord that permissions of the build outputs are owned to root, mostly due to liberal use of sudo
    • sudo is by no means needed. It's easier and cleaner to remove the dependency
    • Resolving relevant problems isn't a goal of this PR, however it's a start on reducing it
  • servicebase images have had the libboost packages replaced with their non-development versions
    • This saves a bit of disk space
    • Future state, we should just statically compile these into the worldserver/authserver binaries
  • Simplified the client-data use in docker
    • This just needs to download and unzip a file, it doesn't need an acore user or anything of the like.

Tests Performed:

  • Built "dev" containers, they come up and work
  • Built "prod" containers, though they do not come up because their section of the docker-compose.yml file is broken (in regards to database)

How to Test the Changes:

  1. Run ./acore.sh docker build
  2. Run ./acore.sh docker start:app
  3. Prune everything
    a. docker-compose --profile app --profile dev --profile prod-app --profile prod down --remove-orphans -v
  4. run ./acore.sh docker prod:build
  5. run ./acore.sh docker prod:up
    a. This will fail due to a bug in the docker-compose.yml file
    b. This is "somewhat" fine, since the prod containers are mainly used exclusively for the acore-docker repo, which hosts its' own docker-compose.yml file

Known Issues and TODO List:

  • The start-to-finish process of building and running the docker containers with the acore.sh dashboard is complex and jumps through many hoops.
    • Though it isn't a goal of this PR, I think in the ideal future state documentation tells users to build and run the containers in ways they're familiar with: the docker cli, or an incredibly thin wrapper (ie, a single layer of abstraction) around the docker cli
  • In this pull request, I was very liberal with TODO comments to audit/review some of the configs for building the docker containers.
    • I intend to work through these comments in order to improve the docker builds by making them simpler and idiomatic
    • Please let me know if I should remove them in order to adhere to style guides

How to Test AzerothCore PRs

When a PR is ready to be tested, it will be marked as [WAITING TO BE TESTED].

You can help by testing PRs and writing your feedback here on the PR's page on GitHub. Follow the instructions here:

http://www.azerothcore.org/wiki/How-to-test-a-PR

REMEMBER: when testing a PR that changes something generic (i.e. a part of code that handles more than one specific thing), the tester should not only check that the PR does its job (e.g. fixing spell XXX) but especially check that the PR does not cause any regression (i.e. introducing new bugs).

For example: if a PR fixes spell X by changing a part of code that handles spells X, Y, and Z, we should not only test X, but we should test Y and Z as well.

@Kitzunu Kitzunu added the run-build Used to trigger the windows/mac/docker and matrix builds label Aug 5, 2023
@podmepodme
Copy link
Contributor

Love the changes! I think this PR could be divided into two parts (refactor and change).

Tested branch, but on: 1. Run ./acore.sh docker build I get an arm architecture related error.

Setup:

  • MacBook Pro 14 M1
  • macOS 13.4.1 22F82 arm64
  • Docker version 24.0.2

image

@michaeldelago
Copy link
Contributor Author

Tested branch, but on: 1. Run ./acore.sh docker build I get an arm architecture related error.

RIP, would probably help if I implemented the thing I had meant to. commit should be in today.

@Kitzunu Kitzunu changed the title feat(Core/Docker): Remove Deno as a dependency from Docker Build refactor(Apps/Docker): Remove Deno as a dependency from Docker Build Aug 5, 2023
@michaeldelago
Copy link
Contributor Author

@podmepodme It should now be fixed. I neglected to consider how the deno install process can get started through the shell script spaghetti.

This has been tested on a ARM64 (t4g) EC2 instance in AWS running Amazon Linux 3, and after installing a system install of Deno, I was able to build and start the containers without issue.

I think this PR could be divided into two parts (refactor and change).

Probably. The problem I ran into is that a lot of the change kind of required a refactor, and then the other refactors were such low hanging fruit that it was hard to resist :p

@Kitzunu
Copy link
Member

Kitzunu commented Aug 5, 2023

It's fine to keep it in one pr =)

@podmepodme
Copy link
Contributor

podmepodme commented Aug 6, 2023

I tested the changes, and this time the build runs fine, but I get the same related error on:
2. Run ./acore.sh docker client-data

EDIT:
Screenshot

image

@michaeldelago
Copy link
Contributor Author

I tested the changes, and this time the build runs fine, but I get the same related error on: 2. Run ./acore.sh docker client-data

Good catch. That didn't happen to me, but maybe I missed something.

@michaeldelago
Copy link
Contributor Author

@podmepodme Ok, I'm pretty sure I've gotten it this time.

@podmepodme
Copy link
Contributor

config.sh, not found on command.

./acore.sh docker client-data

image

@michaeldelago
Copy link
Contributor Author

Oh, sorry. I forgot to document it. the client-data command has been removed from the docker submenu.

It's been replaced with a client-data container that ensures the client-data has been downloaded at when the containers are started. You can remove the client-data step from the procedure when testing this.

@Kitzunu
Copy link
Member

Kitzunu commented Aug 19, 2023

conflict

@Kitzunu
Copy link
Member

Kitzunu commented Aug 22, 2023

Bump

@michaeldelago
Copy link
Contributor Author

Closing this as it has been superseded by #16934

@michaeldelago michaeldelago deleted the prune-docker-deno branch August 26, 2023 14:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bash run-build Used to trigger the windows/mac/docker and matrix builds
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants