search

Just-Some-Bots / MusicBot

:musical_note: The original MusicBot for Discord (formerly SexualRhinoceros/MusicBot)
https://just-some-bots.github.io/MusicBot/
MIT License
3.14k stars 2.36k forks source link

feat: Update Docker and CI #2432

Closed pythoninthegrass closed 1 month ago

pythoninthegrass commented 1 month ago

Description

Original PR at #2431

[!NOTE] This PR includes recent updates from the dev branch. My specific contributions are focused on Docker and CI/CD improvements, as detailed in the commit "feat: update docker and CI/CD".

I did my best to rebase my feature branch against the upstream dev, but ended up coauthoring a handful of commits.

Hoping that since github says the branch can be merged automatically, that should be sufficient 🤞

My key changes include:

These changes are aimed at improving the Docker-related aspects of the project and enhancing the CI/CD pipeline.

Related issues (if applicable)

Closes #2412

BabyBoySnow commented 1 month ago

It seems that the rebase has reverted some of the features in the dev branch

itsTheFae commented 1 month ago

Yeah, this wont fly either. Sorry.
You need to make a clean copy of dev and redo your work here. You cannot rebase master onto dev.

Since this PR is from a branch, you should be able to force-push the clean copy and subsequent changes to that branch and reset the commits of this PR automatically.

pythoninthegrass commented 1 month ago

Don't have the bandwidth for this. Take what you want or not.

glhf ✌️

From: Rhino @.> Sent: Thursday, October 17, 2024 11:42:55 PM To: Just-Some-Bots/MusicBot @.> Cc: pythoninthegrass @.>; Author @.> Subject: Re: [Just-Some-Bots/MusicBot] feat: Update Docker and CI (PR #2432)

@srhinos requested changes on this pull request.

the above comments are all very valid takes. To add on, while I understand your reasoning behind adding them, theres no reason to add the following:

Beyond that, I'd say cut out all the extra fluff added. No one needs a defined docker compose network for this bot. That changes nothing other than increased diff. Less is more

In .github/workflows/docker.ymlhttps://github.com/Just-Some-Bots/MusicBot/pull/2432#discussion_r1805837124:

  • paths:
    • 'Dockerfile*'
    • 'pyproject.toml'
    • 'poetry.lock'
    • 'requirements.txt'
    • '**.py'
    • '**.sh'
    • '.dockerignore'
    • '.env.example'
    • '.github/workflows/**'

what will this actually do

In .github/workflows/docker.ymlhttps://github.com/Just-Some-Bots/MusicBot/pull/2432#discussion_r1805838246:

    • name: Set password by container registry
  • run: |
  • case "${{ env.REGISTRY_URL }}" in
  • "ghcr.io")
  • echo "REGISTRY_PASS=${{ secrets.GITHUB_TOKEN }}" >> $GITHUB_ENV
  • ;;
  • *)
  • if [ -n "${{ secrets.REGISTRY_PASS }}" ]; then
  • echo "REGISTRY_PASS=${{ secrets.REGISTRY_PASS }}" >> $GITHUB_ENV
  • else
  • echo "REGISTRY_PASS secret is not set and registry is not recognized. Exiting..."
  • exit 1
  • fi
  • ;;
  • esac
    • name: Log into container registry
  • if: github.event_name != 'pull_request'
  • uses: @.***
  • with:
  • registry: ${{ env.REGISTRY_URL }}
  • username: ${{ env.REGISTRY_USER }}
  • password: ${{ env.REGISTRY_PASS }}
    • name: Set image name
  • id: image_name
  • run: |
  • if [ -n "${{ env.IMAGE }}" ]; then
  • IMAGE="${{ env.IMAGE }}"
  • else
  • IMAGE=$(grep "LABEL org.opencontainers.image.title" Dockerfile | cut -d'"' -f2)
  • fi
  • echo "IMAGE=$IMAGE" >> $GITHUB_OUTPUT
  • echo "IMAGE=$IMAGE" >> $GITHUB_ENV
    • name: Docker meta
  • id: meta
  • uses: @.***
  • with:
  • images: |
  • ${{ env.REGISTRY_URL }}/${{ env.REGISTRY_USER }}/${{ steps.image_name.outputs.IMAGE }}
  • tags: |
  • type=schedule
  • type=ref,event=branch
  • type=ref,event=pr
  • type=semver,pattern={{version}}
  • type=semver,pattern={{major}}.{{minor}}
  • type=semver,pattern={{major}}
  • type=sha
  • type=raw,value=latest,enable={{is_default_branch}}
    • name: Setup QEMU
  • uses: @.***
    • name: Setup Docker Buildx
  • uses: @.***
    • name: Build and push
  • uses: @.***
  • with:
  • context: .
  • platforms: linux/amd64,linux/arm64/v8
  • push: ${{ github.event_name != 'pull_request' }}
  • tags: ${{ steps.meta.outputs.tags }}
  • labels: ${{ steps.meta.outputs.labels }}
  • cache-from: type=registry,ref=${{ env.REGISTRY_URL }}/${{ env.REGISTRY_USER }}/${{ steps.image_name.outputs.IMAGE }}:buildcache
  • cache-to: type=registry,ref=${{ env.REGISTRY_URL }}/${{ env.REGISTRY_USER }}/${{ steps.image_name.outputs.IMAGE }}:buildcache,mode=max

this is all so insanely complex for a simple task such as uploading a built docker image to github's container registry.

I built this exact workflow in another repo I maintain @ herehttps://github.com/srhinos/primelooter/blob/main/.github/workflows/release.yml, feel free to use it as inspiration as this workflow is really awful.

In Dockerfilehttps://github.com/Just-Some-Bots/MusicBot/pull/2432#discussion_r1805843377:

  • ca-certificates \
  • ffmpeg \
  • gcc \
  • git \
  • libffi \
  • libsodium \
  • opus-dev \
  • && rm -rf /var/cache/apk/*

  • +# pip env vars +ENV PIP_NO_CACHE_DIR=off +ENV PIP_DISABLE_PIP_VERSION_CHECK=on +ENV PIP_DEFAULT_TIMEOUT=100

  • +# don't generate .pyc, enable tracebacks on seg faults +ENV PYTHONDONTWRITEBYTECODE=1

we dont want this enabled. It greatly reduces performance

In Dockerfilehttps://github.com/Just-Some-Bots/MusicBot/pull/2432#discussion_r1805844788:

+# pip env vars +ENV PIP_NO_CACHE_DIR=off +ENV PIP_DISABLE_PIP_VERSION_CHECK=on +ENV PIP_DEFAULT_TIMEOUT=100

these aren't needed as well. No reason to add them

In Dockerfilehttps://github.com/Just-Some-Bots/MusicBot/pull/2432#discussion_r1805845871:

@@ -1,4 +1,6 @@ -FROM python:3.8-alpine +# syntax=docker/dockerfile:1.7.0 + +FROM python:3.8-alpine3.20

is there any reason to deeply specify the alpine version?

In docker-compose.example.ymlhttps://github.com/Just-Some-Bots/MusicBot/pull/2432#discussion_r1805849376:

  • environment:
    • APP_ENV=docker

this isnt really needed unless we're using this in code somewhere.

In docker-compose.example.ymlhttps://github.com/Just-Some-Bots/MusicBot/pull/2432#discussion_r1805850469:

  • networks:
    • musicbot
  • working_dir: /musicbot

  • +networks:

  • musicbot:
  • name: musicbot
  • driver: bridge

none of this networking stuff is needed.

— Reply to this email directly, view it on GitHubhttps://github.com/Just-Some-Bots/MusicBot/pull/2432#pullrequestreview-2376963389, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AA7ILPYLVBL4QWJC6CKP2V3Z4CGU7AVCNFSM6AAAAABQE7DPEGVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDGNZWHE3DGMZYHE. You are receiving this because you authored the thread.Message ID: @.***>

BabyBoySnow commented 1 month ago

@pythoninthegrass fwiw if you have a cellular device with unlimited data I recommend looking into pdanet+ an app which allows you to use your phones data on your computer. (Separate from hotspot)