fix(deploy): register openshell gateway CLI and sync GitOps docs to hp-fleet-gitops#782
Merged
Merged
Conversation
…nerate Previously, regenerating maintenance jobs raised an error if any changed data source still needed an ingest prepare, forcing the operator to queue maintenance jobs first just to refresh JobPackages. Now the pipeline runs ingest_only syncs for those sources, waits for completion, and then builds the maintenance jobs from the refreshed data in a single step. Co-authored-by: Cursor <cursoragent@cursor.com>
The api-entrypoint-openshift.sh readiness loop polled `openshell status` for "Connected", but nothing ever ran `openshell gateway add` to register the gateway with the CLI's local config. The openshell-bootstrap init container stages the mTLS certs but its image only ships the gateway server binary, not the client CLI, so it can't register itself. The api container (which has the CLI) now runs an idempotent `gateway add` using the certs staged by the init container before waiting on status, fixing a CrashLoopBackOff (exit 137) in kartograph-stage. Also documents that stage/prod manifests are now sourced from GitLab fleet-apps (what ArgoCD reads) with hp-fleet-gitops as the Konflux deploy-tag PR target, and adds verification scripts to check the two repos stay in sync and to validate the Vault-backed extraction-runtime secret ESO reads for kartograph-stage. Co-authored-by: Cursor <cursoragent@cursor.com>
hp-gitops-tenants PR #9 repointed the hp-fleet ApplicationSet from GitLab fleet-apps to GitHub hp-fleet-gitops, so ArgoCD now reads the same repo Konflux already opens deploy-tag PRs against. Update the reference Application spec and README accordingly, and flip verify-fleet-apps-kartograph.sh to validate hp-fleet-gitops as canonical (fleet-apps drift is now informational only, and networkpolicy-sticky-runtime.yaml being absent is expected, not missing). Co-authored-by: Cursor <cursoragent@cursor.com>
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Central YAML (base), Organization UI (inherited) Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (2)
🔗 Linked repositories identifiedCodeRabbit considers these linked repositories for cross-repo context during reviews:
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughSummary by CodeRabbit
WalkthroughKartograph stage ArgoCD now targets Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Service as MaintenancePipelineService
participant SyncRunner
participant JobRepo
participant UI as GraphMaintenanceWorkspace
Service->>SyncRunner: ingest_only sync for unprepared sources
SyncRunner-->>Service: completion status
Service->>JobRepo: regenerate pending maintenance jobs
Service-->>UI: maintenance ingest failure text
CWE-829 (Untrusted control sphere): CWE-494 (Download of code without integrity check): CWE-798 (Use of hard-coded credentials): CWE-532 (Insertion of sensitive information into log file): the Vault verifier prints secret diagnostics; only key names, paths, and status text should appear, not secret values or decoded ADC content. 🚥 Pre-merge checks | ✅ 9 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (9 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
✨ Simplify code
Comment |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/api/infrastructure/management/maintenance_pipeline_service.py (1)
205-214: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftAdd a per-source guard before launching ingest-only syncs
src/api/infrastructure/management/maintenance_pipeline_service.py:785-812still creates a freshDataSourceSyncRunfor every source without checking for an existingpending/ingestingrun or taking a lock. Concurrenttrigger/regeneratecalls can enqueue duplicate ingest-only runs for the same data source (CWE-362). Gate this with an active-run check or DB-backed lock so only one ingest can start per source.🤖 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 `@src/api/infrastructure/management/maintenance_pipeline_service.py` around lines 205 - 214, The ingest-only launch path in MaintenancePipelineService can create duplicate DataSourceSyncRun entries for the same source when trigger/regenerate race. Update the per-source sync start flow in the relevant helper that prepares ingest-only runs to first check for an existing active run (pending or ingesting) or acquire a DB-backed lock before creating a new run. Use the existing maintenance pipeline methods such as _prepare_changed_sources_for_maintenance and the sync-run creation logic around the ingest-only source loop to ensure only one ingest can start per data source at a time.
🧹 Nitpick comments (3)
src/api/infrastructure/management/maintenance_pipeline_service.py (2)
208-214: 🩺 Stability & Availability | 🔵 Trivial | 🏗️ Heavy liftUp to 300s synchronous poll inside
regenerate_maintenance_jobs.Calling
_prepare_changed_sources_for_maintenance, which polls_wait_for_sync_runswith_MAINTENANCE_INGEST_WAIT_TIMEOUT_SECONDS = 300.0, blocks the caller for up to 5 minutes. If this is invoked from a synchronous HTTP request handler (e.g.POST .../regenerate-jobs), most reverse-proxy/gateway defaults (30–60s) will time out and retry before this returns, potentially triggering duplicate ingest launches per the race above.Also applies to: 252-277
🤖 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 `@src/api/infrastructure/management/maintenance_pipeline_service.py` around lines 208 - 214, `regenerate_maintenance_jobs` currently blocks on `_prepare_changed_sources_for_maintenance`, which can wait up to 300s via `_wait_for_sync_runs` and time out synchronous request handlers. Refactor the `needs_ingest` path in `regenerate_maintenance_jobs`/`_prepare_changed_sources_for_maintenance` to avoid long in-request polling by moving the wait-and-launch flow to an async background job or returning immediately after enqueuing the ingest, and keep `find_by_knowledge_graph`/`_changed_sources` refresh logic aligned with the new non-blocking flow.
252-277: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate ingest-wait/classify logic.
The timeout →
ValueError,failed→ValueError, unexpected-state →ValueErrorsequence in_prepare_changed_sources_for_maintenance(Lines 266-276) is copy-pasted from_trigger_for_kg(Lines 364-386), just with different error surfaces (raise vs._fail_latest_run). Extract a shared_classify_sync_statuses(statuses) -> Nonehelper (raising) that both call sites wrap appropriately, to avoid the two drifting apart.Also applies to: 363-386
🤖 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 `@src/api/infrastructure/management/maintenance_pipeline_service.py` around lines 252 - 277, The timeout/failed/unexpected-state classification logic in _prepare_changed_sources_for_maintenance is duplicated with _trigger_for_kg and should be centralized. Extract a shared helper around _wait_for_sync_runs that classifies sync statuses once (timeout, any failed, not all ingested) and returns or raises consistently, then have _prepare_changed_sources_for_maintenance and _trigger_for_kg call it and map the result to their respective error handling (_ValueError_ vs _fail_latest_run_). Use the existing symbols _prepare_changed_sources_for_maintenance, _trigger_for_kg, and _wait_for_sync_runs to keep both paths aligned.src/api/tests/unit/management/application/test_maintenance_pipeline_service.py (1)
882-929: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPrepare path fully mocked — no coverage for timeout/failed/unexpected-state branches.
_prepare_changed_sources_for_maintenanceis replaced with a bareAsyncMock(), so the newValueErrorbranches in that method (timeout, failed sync, unexpected terminal state — Lines 268-276 inmaintenance_pipeline_service.py) are untested. Given TDD is mandated for this repo, add at least one test assertingregenerate_maintenance_jobspropagates theValueErrorwhen prepare fails.As per coding guidelines, "OF UTMOST IMPORTANCE: 100% adhere to TDD."
🤖 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 `@src/api/tests/unit/management/application/test_maintenance_pipeline_service.py` around lines 882 - 929, The `regenerate_maintenance_jobs` test fully mocks `_prepare_changed_sources_for_maintenance`, so the new error branches in `MaintenancePipelineService` are not covered. Add a test around `regenerate_maintenance_jobs` that uses `_prepare_changed_sources_for_maintenance` on `MaintenancePipelineService` to raise the `ValueError` for a failed prepare path (timeout/failed/unexpected terminal state) and assert the exception is propagated instead of continuing to `_build_maintenance_jobs`. Keep the existing happy-path test and add this failure-path coverage using the same `svc`, `prepare`, and `job_repo.sync_maintenance_pending_jobs` setup.Source: Coding guidelines
🤖 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 `@scripts/verify-fleet-apps-kartograph.sh`:
- Around line 10-13: The verify script uses a predictable shared /tmp directory
for WORKDIR, which is vulnerable to symlink-planting and path hijacking. Update
the setup in verify-fleet-apps-kartograph.sh so the WORKDIR is created with a
unique temporary directory (for example via mktemp) instead of a fixed default
path, and keep the existing mkdir/cd flow operating on that safely created
directory. Ensure the new temp directory is used consistently by the git
clone/reset logic in this script.
In `@scripts/verify-vault-extraction-runtime.py`:
- Around line 233-239: The stdout summary in the ADC verification path is
exposing secret-derived data from the Vault-sourced JSON, so update the printing
logic around adc parsing to avoid logging any tainted fields. In
verify-vault-extraction-runtime.py, adjust the block that uses adc.get(...) so
it no longer prints project_id (and keep the ADC output fully redacted or
limited to non-sensitive metadata like type only if safe), or add an explicit
justified suppression if that’s the chosen approach.
In `@src/api/scripts/api-entrypoint-openshift.sh`:
- Around line 18-20: The gateway setup in the openshift entrypoint only checks
whether a registration exists, so it can skip re-adding a stale endpoint. Update
the logic around the openshell gateway info / gateway add flow to inspect the
currently registered gateway URL and compare it with
KARTOGRAPH_EXTRACTION_RUNTIME_OPENSHELL_GATEWAY_URL, then call openshell gateway
add again whenever the stored URL differs. Keep the change localized to the
gateway_name/gateway_url registration block so the script re-registers
mismatched endpoints instead of assuming existence is enough.
---
Outside diff comments:
In `@src/api/infrastructure/management/maintenance_pipeline_service.py`:
- Around line 205-214: The ingest-only launch path in MaintenancePipelineService
can create duplicate DataSourceSyncRun entries for the same source when
trigger/regenerate race. Update the per-source sync start flow in the relevant
helper that prepares ingest-only runs to first check for an existing active run
(pending or ingesting) or acquire a DB-backed lock before creating a new run.
Use the existing maintenance pipeline methods such as
_prepare_changed_sources_for_maintenance and the sync-run creation logic around
the ingest-only source loop to ensure only one ingest can start per data source
at a time.
---
Nitpick comments:
In `@src/api/infrastructure/management/maintenance_pipeline_service.py`:
- Around line 208-214: `regenerate_maintenance_jobs` currently blocks on
`_prepare_changed_sources_for_maintenance`, which can wait up to 300s via
`_wait_for_sync_runs` and time out synchronous request handlers. Refactor the
`needs_ingest` path in
`regenerate_maintenance_jobs`/`_prepare_changed_sources_for_maintenance` to
avoid long in-request polling by moving the wait-and-launch flow to an async
background job or returning immediately after enqueuing the ingest, and keep
`find_by_knowledge_graph`/`_changed_sources` refresh logic aligned with the new
non-blocking flow.
- Around line 252-277: The timeout/failed/unexpected-state classification logic
in _prepare_changed_sources_for_maintenance is duplicated with _trigger_for_kg
and should be centralized. Extract a shared helper around _wait_for_sync_runs
that classifies sync statuses once (timeout, any failed, not all ingested) and
returns or raises consistently, then have
_prepare_changed_sources_for_maintenance and _trigger_for_kg call it and map the
result to their respective error handling (_ValueError_ vs _fail_latest_run_).
Use the existing symbols _prepare_changed_sources_for_maintenance,
_trigger_for_kg, and _wait_for_sync_runs to keep both paths aligned.
In
`@src/api/tests/unit/management/application/test_maintenance_pipeline_service.py`:
- Around line 882-929: The `regenerate_maintenance_jobs` test fully mocks
`_prepare_changed_sources_for_maintenance`, so the new error branches in
`MaintenancePipelineService` are not covered. Add a test around
`regenerate_maintenance_jobs` that uses
`_prepare_changed_sources_for_maintenance` on `MaintenancePipelineService` to
raise the `ValueError` for a failed prepare path (timeout/failed/unexpected
terminal state) and assert the exception is propagated instead of continuing to
`_build_maintenance_jobs`. Keep the existing happy-path test and add this
failure-path coverage using the same `svc`, `prepare`, and
`job_repo.sync_maintenance_pending_jobs` setup.
🪄 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: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: fc759d71-6800-41a8-80aa-482faac92337
⛔ Files ignored due to path filters (1)
src/api/uv.lockis excluded by!**/*.lock
📒 Files selected for processing (9)
deploy/README.mddeploy/argocd/kartograph-stage.yamlscripts/verify-fleet-apps-kartograph.shscripts/verify-vault-extraction-runtime.pyspecs/extraction/maintenance-jobs.spec.mdsrc/api/infrastructure/management/maintenance_pipeline_service.pysrc/api/scripts/api-entrypoint-openshift.shsrc/api/tests/unit/management/application/test_maintenance_pipeline_service.pysrc/dev-ui/app/components/graph-management/GraphMaintenanceWorkspace.vue
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
openshift-hyperfleet/architecture(manual)openshift-hyperfleet/hyperfleet-api(manual)openshift-hyperfleet/hyperfleet-sentinel(manual)openshift-hyperfleet/hyperfleet-adapter(manual)openshift-hyperfleet/hyperfleet-broker(manual)
CodeQL (py/clear-text-logging-sensitive-data) flagged two high-severity alerts in scripts/verify-vault-extraction-runtime.py: printing Vault secret key names and ADC type/project_id values pulled directly from the KV payload. Replace those with counts/booleans so no content derived from the secret is ever echoed to stdout, and apply the same redaction to the (unflagged but equivalent) extra-keys note and _validate_adc error messages. Also drops two pre-existing ruff lints (unused variable, redundant f-string) in the same file. Co-authored-by: Cursor <cursoragent@cursor.com>
aredenba-rh
changed the title
…entrypoint - verify-fleet-apps-kartograph.sh: replace the predictable, fixed /tmp/kartograph-gitops-verify default WORKDIR with mktemp -d, closing a symlink-planting risk (CWE-377) since any local user/process could pre-create that path before the script clones/reset --hard's into it. - api-entrypoint-openshift.sh: compare the openshell CLI's stored gateway_endpoint (read from its metadata.json) against KARTOGRAPH_EXTRACTION_RUNTIME_OPENSHELL_GATEWAY_URL before skipping `gateway add`, instead of only checking that a registration exists. Without this, a stale registration surviving in the emptyDir-backed XDG_CONFIG_HOME across container restarts could leave the pod pointed at an endpoint that no longer matches config. Co-authored-by: Cursor <cursoragent@cursor.com>
2 tasks
aredenba-rh
added a commit
that referenced
this pull request
Jul 2, 2026
Reverts commit 2d76a14 (fix(ci): retarget Konflux deploy-tag to fleet-apps on GitLab, #780). That change was correct at the time (2026-06-30): ArgoCD read fleet-apps then. hp-gitops-tenants PR #9 (2026-07-01) repointed ArgoCD's kartograph-stage source at hp-fleet-gitops (GitHub) instead, so the push pipelines' deploy-tag finally task needs to target hp-fleet-gitops again — as it did before #780, via the update-and-push + create-pull-request tasks (plain `git push` + a `curl`-based GitHub PR creation step using the same `.netrc` staged for git auth, since GitHub has no GitLab-style `git push -o merge_request.*` equivalent). Confirmed needed: both kartograph-api-on-push and kartograph-dev-ui-on-push failed at update-deploy-tag on the first push to main after merging #782, because it was still trying (and failing) to push to the GitLab fleet-apps repo. Requires the kartograph-hp-fleet-gitops-auth secret (GitHub PAT with repo + pull_request scope) to exist in the kartograph-tenant Konflux namespace — same requirement as before #780, when this last worked. Co-authored-by: Cursor <cursoragent@cursor.com>
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
2 participants
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
Test Plan
make test-allpassesmake lintpassesmake test-helm(if applicable)