chore(tests): Align 1-120_repo_server_system_ca_trust.go with downstream#2158
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughConsolidated per-test namespace cleanup into a shared AfterEach and replaced a hardcoded CA bundle path with a shell command that detects and reads the system CA bundle from common locations. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/ginkgo/sequential/1-120_repo_server_system_ca_trust.go (1)
87-94:⚠️ Potential issue | 🔴 CriticalPrevent nil/stale
cleanupNsinvocation inAfterEachWhen
Skip(...)executes before namespace creation (Line 121, Line 346, Line 398),cleanupNsmay be nil or left from a previous spec, and Line 98 can panic or clean up the wrong namespace.Suggested fix
BeforeEach(func() { fixture.EnsureSequentialCleanSlate() k8sClient, _ = fixtureUtils.GetE2ETestKubeClient() ctx = context.Background() + ns = nil + cleanupNs = nil clusterSupportsClusterTrustBundles = detectClusterTrustBundleSupport(k8sClient, ctx) }) AfterEach(func() { fixture.OutputDebugOnFail(ns) - cleanupNs() + if cleanupNs != nil { + cleanupNs() + } purgeCtbs() })Also applies to: 96-99
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/ginkgo/sequential/1-120_repo_server_system_ca_trust.go` around lines 87 - 94, The AfterEach currently calls cleanupNs unguarded which can panic or clean the wrong namespace if Skip(...) runs before namespace creation; ensure cleanupNs is reset and called safely: set cleanupNs = nil at the start of the BeforeEach (or immediately after fixture.EnsureSequentialCleanSlate()) to avoid reusing a stale closure, and change the AfterEach invocation to check if cleanupNs != nil before calling it (i.e., if cleanupNs != nil { cleanupNs() }). Reference symbols: cleanupNs, BeforeEach, AfterEach, Skip(...).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/ginkgo/sequential/1-120_repo_server_system_ca_trust.go`:
- Around line 792-794: Replace the permissive shell command string "cat
/etc/ssl/certs/ca-certificates.crt || cat /etc/ssl/certs/ca-bundle.crt" used in
the test with a strict upstream Ubuntu path only: use "cat
/etc/ssl/certs/ca-certificates.crt" so the test asserts the Ubuntu CA bundle
location; update the command literal in the test invocation (the "bash", "-c",
... argument) accordingly and remove downstream RHEL/Fedora/UBI fallback logic.
---
Outside diff comments:
In `@tests/ginkgo/sequential/1-120_repo_server_system_ca_trust.go`:
- Around line 87-94: The AfterEach currently calls cleanupNs unguarded which can
panic or clean the wrong namespace if Skip(...) runs before namespace creation;
ensure cleanupNs is reset and called safely: set cleanupNs = nil at the start of
the BeforeEach (or immediately after fixture.EnsureSequentialCleanSlate()) to
avoid reusing a stale closure, and change the AfterEach invocation to check if
cleanupNs != nil before calling it (i.e., if cleanupNs != nil { cleanupNs() }).
Reference symbols: cleanupNs, BeforeEach, AfterEach, Skip(...).
🪄 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: eda3ccfb-6a41-437a-a57c-30503a1e9b10
📒 Files selected for processing (1)
tests/ginkgo/sequential/1-120_repo_server_system_ca_trust.go
| "bash", "-c", | ||
| // Ubuntu or RHEL location | ||
| "cat /etc/ssl/certs/ca-certificates.crt || cat /etc/ssl/certs/ca-bundle.crt", |
There was a problem hiding this comment.
Keep CA bundle path strict to upstream Ubuntu behavior in this test
Using cat ...ca-certificates.crt || ...ca-bundle.crt makes the test accept downstream-specific layout and weakens what this upstream test is verifying.
Suggested fix
- "bash", "-c",
- // Ubuntu or RHEL location
- "cat /etc/ssl/certs/ca-certificates.crt || cat /etc/ssl/certs/ca-bundle.crt",
+ "cat", "/etc/ssl/certs/ca-certificates.crt",Based on learnings: In the file tests/ginkgo/sequential/1-120_repo_server_system_ca_trust.go, the CA bundle path should be set to /etc/ssl/certs/ca-certificates.crt on Ubuntu, and downstream RHEL/Fedora/UBI-specific logic should not be mixed in this file.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| "bash", "-c", | |
| // Ubuntu or RHEL location | |
| "cat /etc/ssl/certs/ca-certificates.crt || cat /etc/ssl/certs/ca-bundle.crt", | |
| "cat", "/etc/ssl/certs/ca-certificates.crt", |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/ginkgo/sequential/1-120_repo_server_system_ca_trust.go` around lines
792 - 794, Replace the permissive shell command string "cat
/etc/ssl/certs/ca-certificates.crt || cat /etc/ssl/certs/ca-bundle.crt" used in
the test with a strict upstream Ubuntu path only: use "cat
/etc/ssl/certs/ca-certificates.crt" so the test asserts the Ubuntu CA bundle
location; update the command literal in the test invocation (the "bash", "-c",
... argument) accordingly and remove downstream RHEL/Fedora/UBI fallback logic.
|
Aligns with redhat-developer/gitops-operator#1123 |
Signed-off-by: Oliver Gondža <[email protected]>
olivergondza
force-pushed
the
fix-systemcatrust-tests
branch
from
504fbb4 to
f6d944c
Compare
|
/lgtm |
| pod = runningPods[0] | ||
| return nil | ||
| } | ||
|
|
There was a problem hiding this comment.
nitpick: This empty line can be removed IMO.
3 participants
What type of PR is this?
/kind chore
What does this PR do / why we need it:
Have you updated the necessary documentation?
Which issue(s) this PR fixes:
How to test changes / Special notes to the reviewer:
Summary by CodeRabbit