monitoring-plugin: add e2e-management-api Prow job#79673
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds an optional CI e2e job ChangesMonitoring Plugin Management API E2E Test
🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels: 🚥 Pre-merge checks | ✅ 11 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (11 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
sradco
force-pushed
the
monitoring-plugin-management-api-e2e
branch
from
0ba39e4 to
6432570
Compare
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: sradco The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
sradco
force-pushed
the
monitoring-plugin-management-api-e2e
branch
from
6432570 to
d5b7863
Compare
|
@simonpasquier, @jgbernalp, @machadovilaca please review this PR |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
ci-operator/step-registry/monitoring-plugin/tests/management-api/monitoring-plugin-tests-management-api-commands.sh (1)
37-46: ⚡ Quick winConsider a more robust port-forward readiness check.
The 5-second sleep works but doesn't verify the port-forward is actually ready. A polling loop checking port availability would be more reliable.
♻️ Optional: Poll for port readiness
echo "Starting port-forward to localhost:${PORT}..." oc --kubeconfig="${KUBECONFIG}" -n "${NAMESPACE}" \ port-forward "deployment/${DEPLOY_NAME}" "${PORT}:${PORT}" & PF_PID=$! trap "kill ${PF_PID} 2>/dev/null || true; \ oc --kubeconfig=${KUBECONFIG} -n ${NAMESPACE} delete deployment/${DEPLOY_NAME} \ service/${DEPLOY_NAME} --ignore-not-found" EXIT -# Wait for port-forward to be ready -sleep 5 +# Wait for port-forward to be ready +echo "Waiting for port-forward to be ready..." +for i in {1..30}; do + if curl -s -o /dev/null -w "%{http_code}" "http://localhost:${PORT}/health" | grep -q "200\|404"; then + echo "Port-forward is ready" + break + fi + sleep 1 +doneNote: Adjust the health check endpoint as needed for your service.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ci-operator/step-registry/monitoring-plugin/tests/management-api/monitoring-plugin-tests-management-api-commands.sh` around lines 37 - 46, Replace the fixed "sleep 5" with a polling readiness check that verifies the background port-forward (PF_PID) actually opened the local port: after launching oc port-forward for "deployment/${DEPLOY_NAME}" and capturing PF_PID, loop until either a TCP connection to localhost:${PORT} succeeds (e.g., using nc or curl to a health endpoint) or a timeout elapses, and also ensure the loop exits with error if PF_PID dies; keep the existing trap that kills PF_PID and deletes resources but change the readiness logic to poll localhost:${PORT} (or a service health path) instead of a blind sleep.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In
`@ci-operator/step-registry/monitoring-plugin/tests/management-api/monitoring-plugin-tests-management-api-commands.sh`:
- Around line 37-46: Replace the fixed "sleep 5" with a polling readiness check
that verifies the background port-forward (PF_PID) actually opened the local
port: after launching oc port-forward for "deployment/${DEPLOY_NAME}" and
capturing PF_PID, loop until either a TCP connection to localhost:${PORT}
succeeds (e.g., using nc or curl to a health endpoint) or a timeout elapses, and
also ensure the loop exits with error if PF_PID dies; keep the existing trap
that kills PF_PID and deletes resources but change the readiness logic to poll
localhost:${PORT} (or a service health path) instead of a blind sleep.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 559220c1-cdcf-41e5-a65c-c8d2a2ccdc0c
⛔ Files ignored due to path filters (1)
ci-operator/jobs/openshift/monitoring-plugin/openshift-monitoring-plugin-main-presubmits.yamlis excluded by!ci-operator/jobs/**
📒 Files selected for processing (4)
ci-operator/config/openshift/monitoring-plugin/openshift-monitoring-plugin-main.yamlci-operator/step-registry/monitoring-plugin/tests/management-api/monitoring-plugin-tests-management-api-commands.shci-operator/step-registry/monitoring-plugin/tests/management-api/monitoring-plugin-tests-management-api-ref.metadata.jsonci-operator/step-registry/monitoring-plugin/tests/management-api/monitoring-plugin-tests-management-api-ref.yaml
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@ci-operator/step-registry/monitoring-plugin/tests/management-api/monitoring-plugin-tests-management-api-commands.sh`:
- Around line 32-39: The health-check loop that polls
"http://localhost:${PORT}/health" should fail fast if the backend never becomes
ready: after the for-loop that runs curl (the readiness probe loop), add a check
that verifies success (e.g., a boolean/ready flag or re-run curl) and if not
ready print an error like "Backend did not become ready" and exit 1 so tests
(make test-e2e) are not run; apply the same change to the second identical loop
around the later health check so both loops abort the script on timeout instead
of continuing.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 616c3a69-5f18-4dd0-89ec-9584515d3dd7
⛔ Files ignored due to path filters (1)
ci-operator/jobs/openshift/monitoring-plugin/openshift-monitoring-plugin-main-presubmits.yamlis excluded by!ci-operator/jobs/**
📒 Files selected for processing (4)
ci-operator/config/openshift/monitoring-plugin/openshift-monitoring-plugin-main.yamlci-operator/step-registry/monitoring-plugin/tests/management-api/monitoring-plugin-tests-management-api-commands.shci-operator/step-registry/monitoring-plugin/tests/management-api/monitoring-plugin-tests-management-api-ref.metadata.jsonci-operator/step-registry/monitoring-plugin/tests/management-api/monitoring-plugin-tests-management-api-ref.yaml
✅ Files skipped from review due to trivial changes (1)
- ci-operator/step-registry/monitoring-plugin/tests/management-api/monitoring-plugin-tests-management-api-ref.metadata.json
sradco
force-pushed
the
monitoring-plugin-management-api-e2e
branch
from
c9650d7 to
e8df0ad
Compare
Add a new optional presubmit that starts the monitoring-plugin backend directly from source (go run) and runs the Go e2e tests against the live Alerting Management API. Changes: - ci-operator/config: add e2e-management-api test stanza (always_run: false, optional: true) using cluster_profile openshift-org-aws and workflow ipi-aws - ci-operator/jobs: regenerate presubmits via prowgen - step-registry/monitoring-plugin/tests/management-api: new step with commands.sh, ref.yaml, metadata.json, and OWNERS Signed-off-by: Shirly Radco <sradco@redhat.com>
sradco
force-pushed
the
monitoring-plugin-management-api-e2e
branch
from
b0b7d81 to
8c326a9
Compare
|
[REHEARSALNOTIFIER]
Interacting with pj-rehearseComment: Once you are satisfied with the results of the rehearsals, comment: |
|
@sradco: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
1 participant
Summary
e2e-management-apiforopenshift/monitoring-pluginmonitoring-plugin-tests-management-apithat deploys the plugin backend on a provisioned AWS cluster, port-forwards it to localhost, and runs the Go backend integration tests (make test-e2e)/test e2e-management-apiContext
The
test/e2e/directory inopenshift/monitoring-plugincontains Go integration tests for the Alerting Management API that hit the real plugin HTTP server and verifyPrometheusRuleCRDs are created/deleted on the cluster. These tests were not previously wired into any Prow job.This PR fixes that gap by adding the Prow job and step. The step:
monitoring-pluginbackend as a Deployment inopenshift-monitoringlocalhost:9001PLUGIN_URLandKUBECONFIGand runsmake test-e2eRelated: openshift/monitoring-plugin#942
Made with Cursor
Summary by CodeRabbit
This PR adds CI infrastructure in the OpenShift CI repo to run Alerting Management API end-to-end tests for the openshift/monitoring-plugin repository.
What changed (practical terms)
Key behaviors of the new step
Other changes
Impact
Wires previously-unexecuted Alerting Management API integration tests into Prow CI so they run (on demand) against a real plugin HTTP server and a provisioned cluster, validating PrometheusRule CRD creation/deletion end-to-end.