Repository navigation
Conversation
|
🤖 Finished Review · ✅ Success · Started 6:14 AM UTC · Completed 6:34 AM UTC Commit: |
|
CC @FrostyX you would probably be interested to try this out (once we figure out the actual components). Could also be interesting for copr to check |
|
Build succeeded. ✔️ pre-commit SUCCESS in 1m 57s |
ReviewFindingsHigh
Medium
Low
Next steps:
Previous runReviewFindingsLow
Labels: PR adds systemd quadlet deployment configuration files under quadlets/ |
![fullsend-ai-review[bot]](https://avatars.githubusercontent.com/in/3478501?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
fullsend-ai-review
Bot
added
ready-for-merge
Signed-off-by: Cristian Le <git@lecris.dev>
Signed-off-by: Cristian Le <git@lecris.dev>
Signed-off-by: Cristian Le <git@lecris.dev>
Signed-off-by: Cristian Le <git@lecris.dev>
Signed-off-by: Cristian Le <git@lecris.dev>
Signed-off-by: Cristian Le <git@lecris.dev>
|
🤖 Review · ❌ Terminated · Started 6:41 AM UTC · Ended 7:00 AM UTC Commit: |
|
Build succeeded. ✔️ pre-commit SUCCESS in 1m 56s |
|
|
||
| http_flags=( | ||
| --http2 | ||
| ) |
There was a problem hiding this comment.
[medium] logic-error
The --http2 flag is unconditionally added to http_flags, including the plain HTTP code path. Apache's mod_http2 requires TLS for h2. Passing --http2 with --port (plain HTTP) may cause mod_wsgi-express to fail to start or silently ignore the flag.
Suggested fix: Move --http2 into the TLS branch (inside the if [[ -f /secrets/privkey.pem ]] block).
| http_flags+=( | ||
| --https-port "${PORT:-8443}" | ||
| --ssl-certificate-file /secrets/fullchain.pem | ||
| --ssl-certificate-key-file /secrets/privkey.pem |
There was a problem hiding this comment.
[medium] fail-open
Silent fallback to plain HTTP (port 8080) when /secrets/privkey.pem is absent. Previously the server always attempted HTTPS and would fail if certificates were missing (fail-closed). This also affects the existing docker-compose deployment path — a misconfiguration could cause unencrypted traffic.
Suggested fix: Add an explicit warning log in the else branch. Consider restricting the HTTP fallback to dev/local deployments only (e.g., gating on DEPLOYMENT env var).
| vars: | ||
| home_path: "{{ lookup('env', 'HOME') }}" | ||
| packit_service_path: /src | ||
| editable_install: "{{ lookup('env', 'DEPLOYMENT') == 'dev' }}" |
There was a problem hiding this comment.
[low] build-time environment
editable_install reads DEPLOYMENT at Ansible execution time. The existing Dockerfile build path does not set this variable, so editable install only works via the new quadlet build path. Likely intentional but undocumented.
| SERVER_NAME=$(sed -nr 's/^server_name: ([^:]+)(:([0-9]+))?$/\1/p' "$PACKIT_SERVICE_CONFIG") | ||
| HTTPS_PORT=$(sed -nr 's/^server_name: ([^:]+)(:([0-9]+))?$/\3/p' "$PACKIT_SERVICE_CONFIG") | ||
| PORT=$(sed -nr 's/^server_name: ([^:]+)(:([0-9]+))?$/\3/p' "$PACKIT_SERVICE_CONFIG") | ||
|
|
There was a problem hiding this comment.
[low] naming-convention
http_flags uses lowercase while all other substantive variables in the file use UPPER_CASE (ATTEMPTS, SERVER_NAME, PORT, PACKIT_SERVICE_CONFIG).
| vars: | ||
| home_path: "{{ lookup('env', 'HOME') }}" | ||
| packit_service_path: /src | ||
| editable_install: "{{ lookup('env', 'DEPLOYMENT') == 'dev' }}" |
There was a problem hiding this comment.
[low] scope-creep
Adding editable_install to the shared Ansible recipe couples the new quadlet workflow to the existing Dockerfile build pipeline. Existing behavior is preserved (DEPLOYMENT unset = non-editable), but the coupling is undocumented.
fullsend-ai-review
Bot
removed
the
ready-for-merge
|
🤖 Finished Review · ✅ Success · Started 6:41 AM UTC · Completed 7:00 AM UTC Commit: |
LecrisUT
changed the title
|
Hello @LecrisUT, thank you for trying to address the issue. For local deployment (not any public instance), my wish would be:
With these, I should be able to do One additional thing that makes our life very easy when developing Copr is having scripts that do a "oneshot" version of our daemons. For example, we have an action dispatcher and build dispatcher which are services running on the background and they are spawning a new process for every action or build and then processing it. But we also have scripts that take an action ID or a build ID as their input and they process just this one single action or build. Therefore you can easily work on the build pipeiline and you don't even need any servers running. I have no idea how many of these points are applicable to Packit but I can't see a reason why they shouldn't be. |
From playing around with it, I find it useful if you want to make adjustments to it, e.g. I can more easily thinker with a reverse-proxy setup without breaking the whole compose file, or add/remove secrets that I do not use.
Indeed that's what I'm chipping away at next. I am trying to remove each secret requirements and see if it can still work or patch it so that it can. So far I've removed just the SSL requirement and at least it spun up, but I didn't give it any payloads to see if it breaks somewhere further down. I want to do that part next, but would need to figure out how to mock some payloads or mock some other interaction with it.
Hmm, that sounds interesting. From what I can read in the deployment, the only roadblock for this is that the packit service is split into the service (web api) and the workers. Otherwise if we could interface directly with |
|
Hi @LecrisUT, thank you for working on this. My understanding of quadlets is limited, but it looks good so far (at least on surface level). I'll try to follow your progress on this PR to get familiar with new changes as you work on it. I agree with @FrostyX that setting up credentials is probably the biggest pain point when trying to get a deployment of Packit running locally. Personally, it took me hours to get it running and then some more additional hours to be able to send fedmsg payloads to it from the terminal (thanks to @nforro for helping me get this working a couple of months back 🙏). We have an epic relevant to this, but unfortunately, it hasn't been a priority to us lately: #2887 |
| [Container] | ||
| ContainerName=packit-service | ||
| Image=quay.io/packit/service:prod | ||
| User=1024 |
There was a problem hiding this comment.
I do wonder whether this is going to work as-is. Whenever I need to run a local deployment (using the docker-compose setup), I need to open docker-compose.yaml and make sure all instances of user: "1024" are changed from 1024 to the number output by id -u.
The deployment doesn't start properly without it (at least for me).
The goal of such a setup is to have a more composable deployment setup than the docker-compose setup. This makes use of the systemd drop-in configuration files to be able to select the components that you want, e.g.
devbeing a setup that would use the current sources and pick up the live edits. In principle the user would simply run(after setting up systemd to pick up these quadlets file, see
quadlet/README.mdfor more info)TODO: