NO-JIRA: test(e2e/v2): add upsert desired-state-hash E2E tests - #9257
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
mgencur
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
@mgencur: This pull request explicitly references no jira issue. DetailsIn response to this:
Instructions 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 openshift-eng/jira-lifecycle-plugin repository. |
openshift-ci
Bot
added
area/testing
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited) Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Included review availability: Your plan provides up to 12 included reviews per hour; 11 remain after this review. 📝 WalkthroughWalkthroughAdded Suggested reviewers: Merge Risk: ⚪ Minimal · up to This change adds localized end-to-end coverage for the desired-state-hash annotation, with no actionable merge-blocking risk remaining beyond normal checks and review. Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error)
✅ Passed checks (10 passed)
Full details: Stable And Deterministic Test NamesExplanation The new test adds Resolution Use static Ginkgo titles for this test. Move the loop over Full details: Test Structure And QualityExplanation PASS. The added Full details: Topology-Aware Scheduling CompatibilityExplanation PASS — The diff changes only Full details: Ipv6 And Disconnected Network Test CompatibilityExplanation PASS — The added Ginkgo test only reads Deployments through the existing management-cluster client and validates a local annotation with a regular expression. The pull request adds no IPv4 literals, IP parsing, URL construction, DNS lookup, public endpoint, image pull, or external service access. The diff changes only Full details: No-Weak-CryptoExplanation The pull request adds only a regular-expression validation for the Full details: Container-PrivilegesExplanation PASS: The pull request changes only Full details: No-Sensitive-Data-In-LogsExplanation The pull request adds only E2E assertion diagnostics. The new messages print deployment names, namespaces, the annotation key, and an invalid annotation value. The annotation is a SHA-256 desired-state hash, not a password, token, API key, session ID, PII, hostname, or customer data. The diff contains no logging of secrets or sensitive object contents. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Some tools did not complete. Review the errors below. 🔧 golangci-lint (2.12.2)Error: build linters: unable to load custom analyzer "hypershiftlinter": hack/tools/bin/hypershiftlinter.so, plugin: not implemented 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)
test/e2e/v2/tests/control_plane_workloads_test.go (1)
1055-1055: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument the exported test registrar.
DesiredStateHashAnnotationTestis exported but has no Go doc comment. Describe that it registers per-workload checks, skips missing or unmanaged Deployments, and validates the desired-state-hash annotation. Document any panic conditions if registration can panic.As per path instructions, comments on exported functions must describe actual behavior, including panic conditions.
🤖 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 `@test/e2e/v2/tests/control_plane_workloads_test.go` at line 1055, Add a Go doc comment immediately before DesiredStateHashAnnotationTest describing that it registers per-workload checks, skips missing or unmanaged Deployments, and validates the desired-state-hash annotation. Include any actual panic conditions that can occur during registration, without asserting unsupported behavior.Source: Path instructions
🤖 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 `@test/e2e/v2/tests/control_plane_workloads_test.go`:
- Around line 1057-1058: Add a non-empty assertion for the slice returned by
internal.GetControlPlaneWorkloads() before the loop that registers It blocks,
failing fast when workloads is empty. Keep the existing per-workload assertions
unchanged.
---
Nitpick comments:
In `@test/e2e/v2/tests/control_plane_workloads_test.go`:
- Line 1055: Add a Go doc comment immediately before
DesiredStateHashAnnotationTest describing that it registers per-workload checks,
skips missing or unmanaged Deployments, and validates the desired-state-hash
annotation. Include any actual panic conditions that can occur during
registration, without asserting unsupported behavior.
🪄 Autofix
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: bedf559e-c7ab-4f93-b84f-79220c159c0c
📒 Files selected for processing (1)
test/e2e/v2/tests/control_plane_workloads_test.go
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #9257 +/- ##
=======================================
Coverage 46.17% 46.17%
=======================================
Files 783 783
Lines 98406 98406
=======================================
Hits 45442 45442
Misses 49888 49888
Partials 3076 3076
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
|
/approve |
openshift-ci
Bot
added
the
approved
muraee
left a comment
muraee
left a comment
There was a problem hiding this comment.
Looks good — clean, well-structured test that follows the existing patterns in the file.
One minor nit: the managed-by label check hardcodes "hypershift.openshift.io/managed-by" and "control-plane-operator" as raw strings. If there's a constant for that label, it'd be safer to import it so the test doesn't silently skip everything if the label key/value ever changes.
Otherwise, LGTM.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mgencur, muraee The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/pipeline required |
|
Scheduling tests matching the |
|
/retest |
Add E2E v2 test covering the desired-state-hash annotation introduced in support/upsert.ApplyManifest (PR openshift#7713): - DesiredStateHashAnnotationTest: asserts every managed Deployment in the control plane namespace carries a valid 64-char hex desired-state-hash annotation
ec8c287 to
78a6475
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
|
/lgtm |
|
Scheduling tests matching the |
|
/retest |
|
/retest |
3 similar comments
|
/retest |
|
/retest |
|
/retest |
|
/verified by E2E This PR add one E2E test and it passed. |
openshift-ci-robot
added
the
verified
|
@mgencur: This PR has been marked as verified by DetailsIn response to this:
Instructions 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 openshift-eng/jira-lifecycle-plugin repository. |
|
@mgencur: 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. |
3 participants
What this PR does / why we need it:
Add E2E v2 test covering the desired-state-hash annotation introduced in support/upsert.ApplyManifest (PR #7713):
Picking one test from #9236 which was used to verify the feature.
Which issue(s) this PR fixes:
Special notes for your reviewer:
Checklist:
Summary by CodeRabbit