Support artifacts export via Tekton volumeClaimTemplate#73
Conversation
Signed-off-by: avasilev <avasilev@redhat.com>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: 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 |
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughThis PR introduces shared artifact storage across Tekton pipeline tasks using workspaces backed by dynamically provisioned PersistentVolumeClaims, enabling tasks to collaboratively write artifacts to a unified location accessible by an Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant K8s as Kubernetes API
participant PVC as Persistent Volume
participant Task1 as Prepare Task
participant Task2 as Test Task
participant ExportTask as Export-Artifacts Task
User->>K8s: Create PipelineRun with artifacts workspace<br/>volumeClaimTemplate
K8s->>PVC: Provision PVC (artifact_pvc_size)
K8s->>Task1: Mount artifacts workspace
Task1->>PVC: Write artifacts to step/prepare/*
K8s->>Task2: Mount artifacts workspace
Task2->>PVC: Write artifacts to step/test/*
K8s->>ExportTask: Run finally task with complete<br/>artifact tree mounted
ExportTask->>PVC: Read all artifacts (step/prepare/*<br/>+ step/test/*)
ExportTask->>User: Export artifacts
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 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. Review rate limit: 0/1 reviews remaining, refill in 3 minutes and 22 seconds.Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/test_scheduling.py (1)
52-59: Use safe key access in workspace lookup.Using
w.get("name")keeps the assertion path cleaner if a malformed workspace object appears.Suggested tweak
- artifacts_ws = next((w for w in workspaces if w["name"] == "artifacts"), None) + artifacts_ws = next((w for w in workspaces if w.get("name") == "artifacts"), None)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_scheduling.py` around lines 52 - 59, The workspace lookup uses w["name"] which can raise KeyError for malformed workspace objects; update the generator in get_pipelinerun_workspaces test to use w.get("name") when finding the artifacts workspace (e.g., workspaces = get_pipelinerun_workspaces("test-cluster"); artifacts_ws = next((w for w in workspaces if w.get("name") == "artifacts"), None)) so the assertion path stays clean and the subsequent assertions on artifacts_ws and its "volumeClaimTemplate" key remain valid.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@config/forge/workflows/pipeline-full.yaml`:
- Around line 67-83: The post-cleanup finally task is sending job-step:
pre-cleanup instead of job-step: post-cleanup causing it not to run as intended;
update the post-cleanup task's params so job-step is set to post-cleanup (match
the task's intended behavior) and then resolve the Tekton finally-task
parallelism by merging export-artifacts and post-cleanup into a single finally
task (e.g., export-and-cleanup) or otherwise ensure sequencing (you cannot rely
on runAfter between finally tasks), adjusting the taskRef (forge-step) params
and workspace usage accordingly to avoid races on the artifacts workspace.
---
Nitpick comments:
In `@tests/test_scheduling.py`:
- Around line 52-59: The workspace lookup uses w["name"] which can raise
KeyError for malformed workspace objects; update the generator in
get_pipelinerun_workspaces test to use w.get("name") when finding the artifacts
workspace (e.g., workspaces = get_pipelinerun_workspaces("test-cluster");
artifacts_ws = next((w for w in workspaces if w.get("name") == "artifacts"),
None)) so the assertion path stays clean and the subsequent assertions on
artifacts_ws and its "volumeClaimTemplate" key remain valid.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: b64f306e-beba-494b-a645-363ab8c9a3e0
📒 Files selected for processing (12)
Fournos_Design_Document.mdREADME.mdconfig/forge/workflows/pipeline-full.yamlconfig/forge/workflows/pipeline-test-only.yamlconfig/forge/workflows/tasks.yamldev/mock-pipelines/pipeline-full.yamldev/mock-pipelines/pipeline-run-only.yamldev/mock-pipelines/tasks.yamlfournos/core/tekton.pyfournos/settings.pytests/conftest.pytests/test_scheduling.py
Signed-off-by: avasilev <avasilev@redhat.com>
Signed-off-by: avasilev <avasilev@redhat.com>
Signed-off-by: avasilev <avasilev@redhat.com>
Signed-off-by: avasilev <avasilev@redhat.com>
|
/lgtm |
|
/test deploy-fournos-wip |
|
🟢 Test of 'fournos_deploy --project-source' succeeded after 00 hours 10 minutes 24 seconds 🟢 • Link to the test results. • No reports index generated... Test configuration: |
2 participants
Implement support for artifacts export from Forge by mounting a Tekton PipelineRun scoped volumeClaimTemplate.
Closes #15.
Summary by CodeRabbit
Release Notes
New Features
FOURNOS_ARTIFACT_PVC_SIZEenvironment variable (default:5Gi).Documentation