OLS-2926 - adding artifact exporting step and logs finalizer#2011
OLS-2926 - adding artifact exporting step and logs finalizer#2011JoaoFula wants to merge 1 commit into
Conversation
|
Warning Review limit reached
More reviews will be available in 49 minutes and 18 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, 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 include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThe PR adds artifact collection and export at two levels of the CI system: test teardown now gathers OpenShift cluster resources and pod logs locally, and the Tekton pipeline's finally section exports retained logs to Quay for diagnostics and retention. ChangesArtifact Collection and Export
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ 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. Comment |
|
[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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/support/fixtures.ts (1)
114-132: 💤 Low valueConsider capturing init/ephemeral and previous-instance logs.
The loop only iterates
pod.spec?.containers, so logs frominitContainers(and ephemeral debug containers) are skipped. Also,oc logswithout--previousreturns only the current instance — for a container that crashed/restarted during the run (the case you most want to diagnose), the useful logs are in the previous instance.♻️ Possible extension to broaden log coverage
- const containers = (pod.spec?.containers || []).map((c: { name: string }) => c.name); + const containers = [ + ...(pod.spec?.initContainers || []), + ...(pod.spec?.containers || []), + ].map((c: { name: string }) => c.name); for (const container of containers) { const logs = safeOc(['logs', `pod/${podName}`, '-c', container, '-n', OLS_NAMESPACE]); if (logs) { fs.writeFileSync(path.join(podLogsDir, `${podName}-${container}.log`), logs); } + const prevLogs = safeOc([ + 'logs', `pod/${podName}`, '-c', container, '-n', OLS_NAMESPACE, '--previous', + ]); + if (prevLogs) { + fs.writeFileSync(path.join(podLogsDir, `${podName}-${container}-previous.log`), prevLogs); + } }🤖 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 `@tests/support/fixtures.ts` around lines 114 - 132, The pod log collection only iterates pod.spec?.containers so it misses initContainers and ephemeral containers and it only grabs current logs; update the loop that processes pods (podsJson -> pods parsing and the inner container iteration) to also include names from pod.spec?.initContainers and any ephemeral containers (e.g., pod.status?.ephemeralContainerStatuses or equivalent), and when invoking safeOc(['logs', ...]) call it twice per container: once without and once with the '--previous' flag to capture prior crashed instances; keep using podName and container name to generate the same output filenames (e.g., `${podName}-${container}.log`) but ensure you avoid clobbering by appending suffixes like `.previous` or `.ephemeral` when appropriate.
🤖 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 `@tests/support/fixtures.ts`:
- Around line 114-132: The pod log collection only iterates pod.spec?.containers
so it misses initContainers and ephemeral containers and it only grabs current
logs; update the loop that processes pods (podsJson -> pods parsing and the
inner container iteration) to also include names from pod.spec?.initContainers
and any ephemeral containers (e.g., pod.status?.ephemeralContainerStatuses or
equivalent), and when invoking safeOc(['logs', ...]) call it twice per
container: once without and once with the '--previous' flag to capture prior
crashed instances; keep using podName and container name to generate the same
output filenames (e.g., `${podName}-${container}.log`) but ensure you avoid
clobbering by appending suffixes like `.previous` or `.ephemeral` when
appropriate.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 3f97ee76-16e0-4d0f-acbd-ef65671cd2f0
📒 Files selected for processing (3)
.tekton/integration-tests/lightspeed-console-pre-commit.yamltests/support/fixtures.tstests/support/global-teardown.ts
adding artifact exporting step and logs finalizer
JoaoFula
force-pushed
the
add-playwright-artifact-exporting
branch
from
faa7763 to
67c99b2
Compare
1 participant
Summary by CodeRabbit