feat(login): extend --auto-open-browser to work with --web #2115
Conversation
WalkthroughAdds an Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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: celebdor The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
pkg/cli/login/login.go
Outdated
| cmds.Flags().Int32VarP(&o.CallbackPort, "callback-port", "c", o.CallbackPort, "Port for the callback server when using --web. Defaults to a random open port") | ||
|
|
||
| if kcmdutil.FeatureGate("OC_ENABLE_WEB_LOGIN_NO_BROWSER").IsEnabled() { | ||
| cmds.Flags().BoolVar(&o.NoBrowser, "no-browser", o.NoBrowser, "Print the authorization URL instead of opening it in a browser. Only valid with --web.") |
There was a problem hiding this comment.
I think we need some info about why do we need it.
Because If we talk about standard login in OCP; I'm not sure this can be achieved. If yes, we can either re-use the one that was introduced for OIDC login
Line 101 in a25e854
If we talk about OIDC login, it exists
Line 101 in a25e854
There was a problem hiding this comment.
I'm using it mostly for Konflux
openshift-ci
bot
added
the
do-not-merge/hold
pkg/cli/login/login.go
Outdated
| # Log in to the given server through a browser | ||
| oc login localhost:8443 --web --callback-port 8280 | ||
| oc login localhost:8443 --web --callback-port 8280` |
There was a problem hiding this comment.
We don't need changes in here.
pkg/cli/login/login.go
Outdated
| oc login localhost:8443 --web --callback-port 8280 | ||
| oc login localhost:8443 --web --callback-port 8280` | ||
|
|
||
| if kcmdutil.FeatureGate("OC_ENABLE_WEB_LOGIN_NO_BROWSER").IsEnabled() { |
There was a problem hiding this comment.
We don't need a feature gate for this.
pkg/cli/login/login.go
Outdated
| # Log in to the external OIDC issuer through Auth Code + PKCE by starting a local server listening on port 8080 | ||
| oc login localhost:8443 --exec-plugin=oc-oidc --client-id=client-id --extra-scopes=email,profile --callback-port=8080 | ||
| `) | ||
| oc login localhost:8443 --exec-plugin=oc-oidc --client-id=client-id --extra-scopes=email,profile --callback-port=8080` |
There was a problem hiding this comment.
The changes in here is not related.
pkg/cli/login/login.go
Outdated
| `) | ||
|
|
||
| loginExample = templates.Examples(` | ||
| loginExample = func() string { |
celebdor
force-pushed
the
feat/cntrlplane-1538-no-browser-flag
branch
from
e2e29d8 to
7ca31bb
Compare
celebdor
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
pkg/cli/login/login.go (1)
107-110: Address past reviewer feedback about the feature gate.A previous reviewer (ardaguclu) commented that "We don't need a feature gate for this" on the earlier version. The current implementation still uses the feature gate
OC_ENABLE_WEB_LOGIN_NO_BROWSER. If there's a specific reason to keep the feature gate (e.g., for staged rollout or experimental status), please clarify this in the PR discussion or consider removing the gate if it's no longer needed.Additionally, the feature gate string
"OC_ENABLE_WEB_LOGIN_NO_BROWSER"is repeated three times (lines 51, 107, 239), which creates maintenance risk.Consider extracting the feature gate name to a package-level constant:
+const webLoginNoBrowserFeatureGate = "OC_ENABLE_WEB_LOGIN_NO_BROWSER" + var ( loginLong = templates.LongDesc(`Then use it consistently:
- if kcmdutil.FeatureGate("OC_ENABLE_WEB_LOGIN_NO_BROWSER").IsEnabled() { + if kcmdutil.FeatureGate(webLoginNoBrowserFeatureGate).IsEnabled() {Based on past review comments.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (3)
pkg/cli/login/login.go(4 hunks)pkg/cli/login/loginoptions.go(2 hunks)pkg/cli/login/loginoptions_test.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- pkg/cli/login/loginoptions.go
- pkg/cli/login/loginoptions_test.go
🔇 Additional comments (2)
pkg/cli/login/login.go (2)
37-64: LGTM: Clean conditional example generation.The IIFE pattern appropriately generates examples with conditional content based on the feature gate. Since examples are static and evaluated at package initialization, checking the feature gate once at init time is correct.
239-241: LGTM: Validation logic is correct.The validation appropriately enforces that
--no-browsercan only be used with--web, and the feature gate check is consistent with the flag registration. The error message is clear and follows the pattern of other validation errors in this function.
celebdor
force-pushed
the
feat/cntrlplane-1538-no-browser-flag
branch
from
7ca31bb to
6cfbd76
Compare
Extend the existing --auto-open-browser flag to support web login in addition to OIDC login. This provides a consistent interface for controlling browser behavior across both authentication methods. Changes: - Extend --auto-open-browser flag to work with both --web and --exec-plugin - Update flag documentation to clarify default behavior (true for --web, false for OIDC) - Set default to true when using --web (unless explicitly overridden) - Remove --auto-open-browser from OIDC-only validation checks - Update login examples to show --auto-open-browser=false usage - Update test coverage to reflect new behavior When --web is used with --auto-open-browser=false, the authorization URL is printed instead of being automatically opened in a browser. This is useful for environments where browser automation is not desired or possible. Fixes: CNTRLPLANE-1538 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
celebdor
force-pushed
the
feat/cntrlplane-1538-no-browser-flag
branch
from
6cfbd76 to
b6d05d7
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
pkg/cli/login/loginoptions_test.go (1)
491-547: Consider adding negative test cases and clarify test case naming.The validation tests cover valid flag combinations well, but consider:
Missing negative tests: All cases expect no error. If certain flag combinations should be invalid (e.g.,
--auto-open-browserwithout--weband without OIDC flags), add tests for those.Misleading test case name (line 511): The comment says "will default to true" but explicitly sets
autoOpenBrowser: false. This test case appears to verify that validation passes when the flag isn't set (which would default at runtime). Consider clarifying the name to reflect what's actually being tested, e.g., "valid: --web when auto-open-browser will use runtime default".
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (3)
pkg/cli/login/login.go(4 hunks)pkg/cli/login/loginoptions.go(1 hunks)pkg/cli/login/loginoptions_test.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- pkg/cli/login/login.go
- pkg/cli/login/loginoptions.go
🔇 Additional comments (1)
pkg/cli/login/loginoptions_test.go (1)
4-4: LGTM!The new imports (
bytes,net/url,strings) are appropriately used in the test functions added below.Also applies to: 11-11, 13-13
| func TestAutoOpenBrowserURLHandling(t *testing.T) { | ||
| testCases := []struct { | ||
| name string | ||
| autoOpenBrowser bool | ||
| expectedOutputRegex string | ||
| shouldNotContain string | ||
| }{ | ||
| { | ||
| name: "with --auto-open-browser=false prints URL without opening", | ||
| autoOpenBrowser: false, | ||
| expectedOutputRegex: "Please visit the following URL in your browser: https://example.com/oauth/authorize\\?test=1\nThe callback server is listening and will receive the authentication response.\n", | ||
| shouldNotContain: "Opening login URL in the default browser", | ||
| }, | ||
| { | ||
| name: "with --auto-open-browser=true shows opening message", | ||
| autoOpenBrowser: true, | ||
| expectedOutputRegex: "Opening login URL in the default browser: https://example.com/oauth/authorize\\?test=1\n", | ||
| shouldNotContain: "Please visit the following URL", | ||
| }, | ||
| } | ||
|
|
||
| for _, tc := range testCases { | ||
| t.Run(tc.name, func(t *testing.T) { | ||
| // Capture output | ||
| outBuf := &bytes.Buffer{} | ||
| streams := genericiooptions.IOStreams{ | ||
| In: strings.NewReader(""), | ||
| Out: outBuf, | ||
| ErrOut: &bytes.Buffer{}, | ||
| } | ||
|
|
||
| options := &LoginOptions{ | ||
| WebLogin: true, | ||
| OIDCAutoOpenBrowser: tc.autoOpenBrowser, | ||
| IOStreams: streams, | ||
| } | ||
|
|
||
| // Create a test login URL handler that matches the actual implementation | ||
| loginURLHandler := func(u *url.URL) error { | ||
| loginURL := u.String() | ||
| if !options.OIDCAutoOpenBrowser { | ||
| fmt.Fprintf(options.Out, "Please visit the following URL in your browser: %s\n", loginURL) | ||
| fmt.Fprintf(options.Out, "The callback server is listening and will receive the authentication response.\n") | ||
| return nil | ||
| } else { | ||
| fmt.Fprintf(options.Out, "Opening login URL in the default browser: %s\n", loginURL) | ||
| // Don't actually call browser.OpenURL in tests | ||
| return nil | ||
| } | ||
| } | ||
|
|
||
| // Test the handler with a test URL | ||
| testURL, _ := url.Parse("https://example.com/oauth/authorize?test=1") | ||
| err := loginURLHandler(testURL) | ||
|
|
||
| if err != nil { | ||
| t.Errorf("unexpected error: %v", err) | ||
| } | ||
|
|
||
| output := outBuf.String() | ||
|
|
||
| // Check expected output using regex | ||
| matched, regexErr := regexp.MatchString(tc.expectedOutputRegex, output) | ||
| if regexErr != nil { | ||
| t.Fatalf("regex error: %v", regexErr) | ||
| } | ||
| if !matched { | ||
| t.Errorf("output did not match expected pattern.\nExpected pattern: %s\nActual output: %s", tc.expectedOutputRegex, output) | ||
| } | ||
|
|
||
| // Check that certain strings are not present | ||
| if tc.shouldNotContain != "" && strings.Contains(output, tc.shouldNotContain) { | ||
| t.Errorf("output should not contain '%s', but got: %s", tc.shouldNotContain, output) | ||
| } | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Refactor test to verify actual implementation rather than a mock.
This test defines a local loginURLHandler function (lines 587-598) that duplicates the expected implementation logic, then tests that mock function instead of the actual LoginOptions methods. This approach has significant limitations:
- If the real implementation in
loginoptions.gochanges, this test won't catch regressions - The test validates its own mock logic, not production code
- Provides false confidence since it doesn't exercise actual code paths
Consider restructuring to:
- Call the actual
LoginOptionsmethod that handles web login - Mock the browser-opening dependency (e.g., inject a test
browser.OpenURLimplementation) - Verify behavior through the actual code path rather than reimplementing the logic in the test
Example approach:
-// Create a test login URL handler that matches the actual implementation
-loginURLHandler := func(u *url.URL) error {
- loginURL := u.String()
- if !options.OIDCAutoOpenBrowser {
- fmt.Fprintf(options.Out, "Please visit the following URL in your browser: %s\n", loginURL)
- fmt.Fprintf(options.Out, "The callback server is listening and will receive the authentication response.\n")
- return nil
- } else {
- fmt.Fprintf(options.Out, "Opening login URL in the default browser: %s\n", loginURL)
- // Don't actually call browser.OpenURL in tests
- return nil
- }
-}
-
-// Test the handler with a test URL
-testURL, _ := url.Parse("https://example.com/oauth/authorize?test=1")
-err := loginURLHandler(testURL)
+// Mock the browser opener to avoid actually opening a browser
+options.browserOpener = func(url string) error {
+ return nil // no-op in tests
+}
+
+// Call the actual method that handles web login URL
+// (adjust based on actual method name in LoginOptions)
+err := options.handleWebLoginURL("https://example.com/oauth/authorize?test=1")This ensures the test validates actual production behavior.
Committable suggestion skipped: line range outside the PR's diff.
|
@celebdor: The following test failed, say
Full PR test history. Your PR dashboard. 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 kubernetes-sigs/prow repository. I understand the commands that are listed here. |
| } | ||
| } | ||
|
|
||
| func TestAutoOpenBrowserURLHandling(t *testing.T) { |
There was a problem hiding this comment.
We can remove this test function. Because it seems that it does not exercise the behavior of oc command (it exercises its own custom loginhandler).
2 participants
Summary
• Extend the existing --auto-open-browser flag to support web login in addition to OIDC login
• Provides a consistent interface for controlling browser behavior across both authentication methods
• When --web is used with --auto-open-browser=false, the authorization URL is printed instead of being automatically opened
• Useful for environments where browser automation is not desired or possible (e.g., Konflux)
Changes
• Extended --auto-open-browser flag to work with both --web and --exec-plugin
• Updated flag documentation to clarify default behavior (true for --web, false for OIDC)
• Set default to true when using --web (unless explicitly overridden)
• Removed --auto-open-browser from OIDC-only validation checks
• Updated login examples to show --auto-open-browser=false usage
• Updated test coverage to reflect new behavior
Test plan
Fixes: CNTRLPLANE-1538
🤖 Generated with Claude Code