TRT-2652: Allow info and list tests commands to work without KUBECONFIG#458
TRT-2652: Allow info and list tests commands to work without KUBECONFIG#458petr-muller wants to merge 1 commit intoopenshift:mainfrom
Conversation
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
WalkthroughFramework initialization in main.go is refactored to separate cluster-independent setup ( Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 1 warning, 1 inconclusive)
✅ Passed checks (9 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
/test all |
|
[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)
openshift-tests/ccm-aws-tests/main.go (1)
159-185: HardeninitFrameworkCluster()with an explicitsync.Onceguard for defense in depth.The
framework.AfterReadingAllFlags()call has multiple non-idempotent side effects (image initialization, gomega configuration, spec validation, token generation). While currently guarded byOneTimeTasksemantics inAddBeforeAll, making this function explicitly one-time idempotent will prevent regressions if additional call paths are introduced in the future.The proposed refactoring also improves KUBECONFIG validation with
strings.TrimSpace()to handle whitespace-only edge cases.♻️ Proposed hardening
import ( "context" "fmt" "os" "path/filepath" + "sync" "strings" @@ var ( // testContext is the global test context that is used to store the test configuration. testContext = &framework.TestContext + initFrameworkClusterOnce sync.Once + initFrameworkClusterErr error ) @@ func initFrameworkCluster() error { - if len(os.Getenv("KUBECONFIG")) == 0 { - return fmt.Errorf("KUBECONFIG is empty. Set the KUBECONFIG environment variable") - } - - // Load kube client config and set the host variable for kubectl - testContext.KubeConfig = os.Getenv("KUBECONFIG") - clientConfig := clientcmd.NewNonInteractiveDeferredLoadingClientConfig( - &clientcmd.ClientConfigLoadingRules{ - ExplicitPath: testContext.KubeConfig, - }, - &clientcmd.ConfigOverrides{}, - ) - cfg, err := clientConfig.ClientConfig() - if err != nil { - return fmt.Errorf("failed to get client config: %w", err) - } - testContext.Host = cfg.Host - - // After reading all flags, this will configure the test context, and needs to be - // called once by framework to avoid re-configuring the test context, and leading - // to issues in Ginkgo phases (PhaseBuildTopLevel, PhaseBuildTree, PhaseRun), - // such as: 'cannot clone suite after tree has been built' - framework.AfterReadingAllFlags(testContext) - - return nil + initFrameworkClusterOnce.Do(func() { + if len(strings.TrimSpace(os.Getenv("KUBECONFIG"))) == 0 { + initFrameworkClusterErr = fmt.Errorf("KUBECONFIG is empty. Set the KUBECONFIG environment variable") + return + } + + // Load kube client config and set the host variable for kubectl + testContext.KubeConfig = os.Getenv("KUBECONFIG") + clientConfig := clientcmd.NewNonInteractiveDeferredLoadingClientConfig( + &clientcmd.ClientConfigLoadingRules{ + ExplicitPath: testContext.KubeConfig, + }, + &clientcmd.ConfigOverrides{}, + ) + cfg, err := clientConfig.ClientConfig() + if err != nil { + initFrameworkClusterErr = fmt.Errorf("failed to get client config: %w", err) + return + } + testContext.Host = cfg.Host + + framework.AfterReadingAllFlags(testContext) + }) + return initFrameworkClusterErr }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openshift-tests/ccm-aws-tests/main.go` around lines 159 - 185, Wrap the non-idempotent setup in initFrameworkCluster() with a sync.Once to ensure framework.AfterReadingAllFlags(testContext) is executed only once even if initFrameworkCluster() is called multiple times; also tighten KUBECONFIG validation by using strings.TrimSpace(os.Getenv("KUBECONFIG")) and treat empty/whitespace-only values as invalid (return error). Locate the calls to testContext.KubeConfig, clientcmd.NewNonInteractiveDeferredLoadingClientConfig, and framework.AfterReadingAllFlags to implement the sync.Once guard and updated validation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@openshift-tests/ccm-aws-tests/main.go`:
- Around line 159-185: Wrap the non-idempotent setup in initFrameworkCluster()
with a sync.Once to ensure framework.AfterReadingAllFlags(testContext) is
executed only once even if initFrameworkCluster() is called multiple times; also
tighten KUBECONFIG validation by using
strings.TrimSpace(os.Getenv("KUBECONFIG")) and treat empty/whitespace-only
values as invalid (return error). Locate the calls to testContext.KubeConfig,
clientcmd.NewNonInteractiveDeferredLoadingClientConfig, and
framework.AfterReadingAllFlags to implement the sync.Once guard and updated
validation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: e1830bbc-23a8-4392-baec-1ed802784f6f
📒 Files selected for processing (1)
openshift-tests/ccm-aws-tests/main.go
petr-muller
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
@petr-muller: 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
removed
the
do-not-merge/work-in-progress
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
/test all |
Split initFrameworkForTests() into initFrameworkDefaults() (runs at startup without KUBECONFIG) and initFrameworkCluster() (deferred to AddBeforeAll, requires KUBECONFIG). This enables workflows that need to discover or list tests without a live cluster connection, such as the openshift-tests "list all-tests" command. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
petr-muller
force-pushed
the
fix-info-without-kubeconfig
branch
from
50ed490 to
66a64ba
Compare
|
/test all |
|
/test unit |
|
/test e2e-aws-ovn |
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
|
@petr-muller: 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. |
petr-muller
changed the title
|
@petr-muller: This pull request references TRT-2652 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. |
2 participants
Summary
Split
initFrameworkForTests()into two functions:initFrameworkDefaults()— sets cluster-independent framework defaults (provider, flags, OS distro). Runs at startup, no KUBECONFIG needed.initFrameworkCluster()— loads kubeconfig, sets host, callsAfterReadingAllFlags. Deferred toAddBeforeAll, only runs when tests are actually executed.This enables workflows that need to discover or list tests from ccm-aws-tests without a live cluster connection, such as the
openshift-tests list all-testscommand being added in openshift/origin#31105.Related
openshift-tests list all-testscommand, which callsinfoandlist testson all payload extension binaries without cluster access🤖 Generated with Claude Code
Summary by CodeRabbit