CNTRLPLANE-3428: tls: introduce narrow target types and migrate test functions#31160
CNTRLPLANE-3428: tls: introduce narrow target types and migrate test functions#31160gangwgr wants to merge 3 commits into
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: automatic mode |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughRefactors tests in ChangesTLS verification target refactor (single cohesive cohort)
🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels: Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (1 error, 1 warning)
✅ Passed checks (10 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
openshift-ci
Bot
added
the
approved
gangwgr
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
@gangwgr: This pull request references CNTRLPLANE-3428 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "5.0.0" version, but no target version was set. 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. |
![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
🤖 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/extended/tls/tls_observed_config.go`:
- Around line 109-118: configMapTargets is missing controlPlane: true on several
control-plane entries which makes guestSideConfigMapTargets() incorrectly
include ConfigMaps for HyperShift guests; update the configMapTargets slice to
add controlPlane: true to the entries for "openshift-controller-manager",
"openshift-kube-apiserver" (kube-apiserver-operator-config),
"openshift-apiserver", "openshift-kube-controller-manager", and
"openshift-kube-scheduler" so it matches observedConfigTargets/serviceTargets
and prevents guest-side verification against non-existent guest ConfigMaps.
🪄 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: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 95cfc937-791b-4fb6-9740-ad634597701c
📒 Files selected for processing (1)
test/extended/tls/tls_observed_config.go
openshift-merge-bot
Bot
added
the
ready-for-human-review
gangwgr
force-pushed
the
tls-dedup-helpers-3
branch
from
40203f2 to
a015595
Compare
|
Scheduling required tests: |
gangwgr
force-pushed
the
tls-dedup-helpers-3
branch
2 times, most recently
from
17f6ed3 to
4034867
Compare
|
Scheduling required tests: |
|
|
gangwgr
force-pushed
the
tls-dedup-helpers-3
branch
2 times, most recently
from
2763ef2 to
fe8c1ed
Compare
|
/test verify |
| // running on a HyperShift guest cluster. | ||
| // Pre-compute guest-side target lists so the filter functions are | ||
| // called once rather than on every config-change verification. | ||
| guestObservedCfg := guestSideObservedConfigTargets() |
There was a problem hiding this comment.
How is this change related to "tls: skip HyperShift management setup gracefully"?
This change (and other lines below) belongs more under "tls: migrate to narrow target types" commit (where the functions are defined).
Also, it might be even more transparent to move it under a separate commit.
gangwgr
force-pushed
the
tls-dedup-helpers-3
branch
from
fe8c1ed to
5cd1345
Compare
Replace the monolithic tlsTarget struct and targets slice with purpose-specific narrow types (observedConfigTarget, configMapTarget, deploymentEnvVarTarget, serviceTarget, deploymentRolloutTarget). Each test loop now iterates over its own typed slice, removing the field-presence checks that previously filtered the unified list. Function signatures are updated to accept the narrow types, making data dependencies explicit at the call site. Old helper functions (targetClusterOperators, guestSideTargets, guestSideClusterOperators) are replaced by the pre-built clusterOperatorNames slice and typed guestSide*() filter functions. Guest-side target lists are pre-computed once at Describe level.
gangwgr
force-pushed
the
tls-dedup-helpers-3
branch
from
5cd1345 to
7e184e8
Compare
|
|
||
| setupHyperShiftManagement := func() { | ||
| if os.Getenv("HYPERSHIFT_MANAGEMENT_CLUSTER_KUBECONFIG") == "" || os.Getenv("HYPERSHIFT_MANAGEMENT_CLUSTER_NAMESPACE") == "" { | ||
| g.Skip("HYPERSHIFT_MANAGEMENT_CLUSTER_KUBECONFIG and HYPERSHIFT_MANAGEMENT_CLUSTER_NAMESPACE must be set for config-change tests on HyperShift") |
There was a problem hiding this comment.
Should not this fail instead?
There was a problem hiding this comment.
No, it should skip only
There was a problem hiding this comment.
The message says "must be set" which gives the impression it is required when setupHyperShiftManagement is invoked. In which test cases are both envs expected to be set?
|
Scheduling required tests: |
|
/retest-required |
Defer HyperShift management-cluster initialization to the individual config-change tests that actually need it, instead of running it unconditionally in BeforeEach. Tests that only read guest-cluster state (annotation restoration, servingInfo restoration) no longer require HYPERSHIFT_MANAGEMENT_CLUSTER_KUBECONFIG to be set. The new setupHyperShiftManagement() helper checks for the required env vars and calls g.Skip when they are absent, so config-change tests are skipped cleanly rather than failing with a hard error.
gangwgr
force-pushed
the
tls-dedup-helpers-3
branch
from
7e184e8 to
9e326b3
Compare
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: gangwgr, ingvagabund 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 |
|
Scheduling required tests: |
|
/test e2e-metal-ipi-ovn-ipv6 |
|
@gangwgr: The following tests failed, say
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. |
|
Job Failure Risk Analysis for sha: 9e326b3
|
3 participants
Introduce observedConfigTarget, configMapTarget, deploymentEnvVarTarget, serviceTarget, and deploymentRolloutTarget types, each carrying only the fields their respective test function reads.
Migrate all test function signatures and Ginkgo It loops to use the narrow types, removing the monolithic tlsTarget struct and unified targets slice.
Move HyperShift management cluster setup into a lazy setupHyperShiftManagement helper that skips tests gracefully when HYPERSHIFT_MANAGEMENT_CLUSTER_KUBECONFIG or HYPERSHIFT_MANAGEMENT_CLUSTER_NAMESPACE are not seen
Summary by CodeRabbit