feat: Migrate package manager to uv#1553
Conversation
| environment: | ||
| HASURA_GRAPHQL_DATABASE_URL: postgres://${POSTGRES_USER:-ash}:${POSTGRES_PASSWORD:-pokemon}@db:5432/${POSTGRES_DB:-pokeapi} | ||
| HASURA_GRAPHQL_ENABLE_CONSOLE: "true" | ||
| HASURA_GRAPHQL_ENABLE_CONSOLE: "false" |
There was a problem hiding this comment.
I moved all the dev settings to the -dev compose file and left this as barebones base compose file, the graphql console is not really a recommended setting to be enabled to be in prod/ is a local/dev based setting so figured overriding it in the dev compose file is a more suitable location for it
is there any particular reason for us to have it globally set to true I am not that familiar with graphql but shouldnt the console be enabled in a much more limited manner or does it not matter in our case?
|
I want to review this PR, don't merge please. I'm slow I know, bear with me |
| with: | ||
| submodules: recursive | ||
| - name: Build | ||
| - name: Build and start services |
There was a problem hiding this comment.
This is not building anymore, just starting the services.
There was a problem hiding this comment.
docker compose up does build the image too if it dosent exist (which it dosent when this task first runs) but ill also add a --build flag to be explicit
| docker compose exec -T db psql -U ash -d postgres -c "DROP DATABASE pokeapi WITH (FORCE);" | ||
| docker compose exec -T db psql -U ash -d postgres -c "CREATE DATABASE pokeapi;" | ||
| - name: Import database | ||
| run: docker compose exec -T db pg_restore -U ash -d pokeapi < pokeapi.dump |
There was a problem hiding this comment.
Can we put back -h localhost? I like the commands to be descriptive, so that anyone can see the pipeline and use it as a piece of documentation.
There was a problem hiding this comment.
the default behavior for dump and restore for postgresql isnt localhost or the local loopback address instead it uses the unix socket (file based) which is faster without the tcp/ip overhead I can force it to use localhost if required though but imo there is no particular reason to go through localhost
previously the docker invocation was using -u postgres (os system user) and the the command was using -u ash which likely touched upon this auth permission rule with unix socket file perms vs tcp/ip bypass https://www.postgresql.org/docs/current/auth-trust.html due to differing users in the command and db role access
Local TCP/IP connections are not restricted by file-system permissions
docker compose exec -T -u postgres db sh -c "cd /tmp && pg_restore -h localhost -U ash -d pokeapi pokeapi.dump"
in the new workflow the commands run as the container default execution user so we dont run into any permission misalignments for file r/w
| RUN apk add --no-cache --virtual .build-deps gcc g++ musl-dev \ | ||
| postgresql-dev binutils rust cargo && \ | ||
| python3 -m pip install -r requirements.txt --no-cache-dir | ||
| RUN --mount=type=cache,target=/root/.cache/uv \ |
There was a problem hiding this comment.
same here, let's remove the cache mount and add it later in a separate PR.
There was a problem hiding this comment.
lets leave this cache here? since its a uv or a requirements dependency cache ensuring uv dosent have to reach out to pypi and redownload all packages if there is a change in pyproject or uv lock its recommended in their docs too https://docs.astral.sh/uv/guides/integration/docker/#caching
Ill remove the ci/cd or workflow layer image caching mentioned above though for a future pr
|
was on vacation last week 🏖️ so changes/followup were bit delayed |
…ile stylistic change
FallenDeity
force-pushed
the
feat/migrate-to-uv
branch
from
3d5be8e to
5c44e0f
Compare
4 participants
Change description
First set of pr for migrating and adopting modern python tooling ecosystem, this pr attempts to migrate from pip to uv for package management as discussed in #1391 & #1521 it breaks down the changes @phalt initially made to smaller more iterative parts
cc: @phalt @Naramsim
AI coding assistance disclosure
Used a bit for reviewing the changes and coverage if I missed any transitive command update
Contributor check list