Use split candlepin containers for integration tests#22
Open
Lorquas wants to merge 1 commit into
Open
Conversation
Reviewer's guide (collapsed on small PRs)Reviewer's GuideUpdates the integration test GitHub Actions workflow to use split Candlepin app and database containers instead of the legacy monolithic image, and adjusts service options and readiness checks accordingly. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- Since the readiness probe now uses
curl -kto bypass TLS verification, consider adding a brief inline comment explaining why insecure mode is acceptable here (self-signed test certs, PQC setup, etc.) to prevent future confusion or accidental reuse of this pattern in non-test workflows.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Since the readiness probe now uses `curl -k` to bypass TLS verification, consider adding a brief inline comment explaining why insecure mode is acceptable here (self-signed test certs, PQC setup, etc.) to prevent future confusion or accidental reuse of this pattern in non-test workflows.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Replace the monolithic candlepin-unofficial image with split containers: - candlepin-app (ghcr.io/candlepin/candlepin-app) for the application - candlepin-db (ghcr.io/candlepin/candlepin-db) as a PostgreSQL service The DB service is named "postgres" to match the upstream Candlepin image's db_host configuration, eliminating the need for runtime config overrides. Assisted-by: Claude (Cursor)
ea66d9e to
e1e10b2
Compare
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- The new DB service is named
postgresbut the description mentionsDB_HOST=candlepin-db; consider either renaming the service tocandlepin-dbor settingDB_HOST=postgresin the candlepin app service to keep the configuration consistent and avoid connection issues. - The hard-coded HTTPS URL (
https://candlepin:8443/candlepin/status) in the readiness check could be extracted into an environment variable or matrix value so future protocol/port changes don’t require editing the workflow logic.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new DB service is named `postgres` but the description mentions `DB_HOST=candlepin-db`; consider either renaming the service to `candlepin-db` or setting `DB_HOST=postgres` in the candlepin app service to keep the configuration consistent and avoid connection issues.
- The hard-coded HTTPS URL (`https://candlepin:8443/candlepin/status`) in the readiness check could be extracted into an environment variable or matrix value so future protocol/port changes don’t require editing the workflow logic.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Contributor
|
Integration tests fails, because the candlepin si not deployed. This steps fail: https://github.com/candlepin/rhsm-dnf5-plugins/blob/main/.github/workflows/integration-tests.yml#L58. It seems that candlepin was not started there properly. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Updates the integration test workflow to use the new split Candlepin container images (
candlepin-app+candlepin-db) instead of the monolithiccandlepin-unofficialimage.Changes
ghcr.io/candlepin/candlepin-unofficial:latestwithghcr.io/candlepin/candlepin-app:latest+ghcr.io/candlepin/candlepin-db:latestas GitHub Actions servicesDB_HOST: candlepin-dbon the app service so it connects to the database via the Docker service network--privilegedfrom the candlepin service — the new image runs Tomcat in foreground without systemd, so privileged mode is no longer neededhttp://candlepin:8080tohttps://candlepin:8443(the new image has working HTTPS with ML-DSA PQC certificates)Why
The monolithic
candlepin-unofficial:latestimage is no longer maintained and does not build against Candlepin 4.8+ (JDK and dependency changes). The split images are built from source on UBI 10 with JDK 25 and include pre-baked test data (owners, products, yum repos) that these integration tests require.Depends on
Test plan
split_image_buildworkflow in candlepin-container-unofficial to publish images to ghcr.iorhc connect --username admin --password admin --organization donaldducksucceeds