Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[27.x backport] vendor.mod: put github.com/pkg/browser in the right group #5408

Merged
merged 1 commit into from
Sep 5, 2024

Conversation

thaJeztah
Copy link
Member

commit fcfdd7b (#5344) introduced github.com/pkg/browser as a direct dependency, but it ended up in the group for indirect dependencies.

commit fcfdd7b introduced github.com/pkg/browser
as a direct dependency, but it ended up in the group for indirect dependencies.

Signed-off-by: Sebastiaan van Stijn <[email protected]>
(cherry picked from commit 1b8180a)
Signed-off-by: Sebastiaan van Stijn <[email protected]>
@thaJeztah thaJeztah added status/2-code-review kind/refactor PR's that refactor, or clean-up code labels Sep 5, 2024
@thaJeztah thaJeztah added this to the 27.2.1 milestone Sep 5, 2024
@thaJeztah thaJeztah self-assigned this Sep 5, 2024
@codecov-commenter
Copy link

codecov-commenter commented Sep 5, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 59.77%. Comparing base (d0c1a80) to head (869df10).
Report is 3 commits behind head on 27.x.

Additional details and impacted files
@@           Coverage Diff           @@
##             27.x    #5408   +/-   ##
=======================================
  Coverage   59.77%   59.77%           
=======================================
  Files         345      345           
  Lines       23405    23405           
=======================================
  Hits        13990    13990           
  Misses       8445     8445           
  Partials      970      970           

@thaJeztah
Copy link
Member Author

Flaky test?

=== Failed
=== FAIL: e2e/registry TestOauthLogin (1.00s)
    login_test.go:28: terminating PID 10523
    login_test.go:33: assertion failed: expression is false: strings.Contains(string(output), "USING WEB-BASED LOGIN"): Failed to start web-based login - falling back to command line login...
        
        Log in with your Docker ID or email address to push and pull images from Docker Hub. If you don't have a Docker ID, head over to https://hub.docker.com/ to create one.
        You can log in with your password or a Personal Access Token (PAT). Using a limited-scope PAT grants better security and is required for organizations using SSO. Learn more at https://docs.docker.com/go/access-tokens/
        
        Username: 

@laurazard
Copy link
Member

laurazard commented Sep 5, 2024

Flaky due to it failing to reach login.docker.com 😅

Failed to start web-based login - falling back to command line login...

I have a branch somewhere with a refactor/more e2e tests here, so I'll make sure this is more resilient. It's not testing the full login flow, just that the oauth login is triggered by a docker login, so if it fallbacks to regular login that's fine.

@thaJeztah
Copy link
Member Author

I just noticed that we're swallowing the error there, which ... probably is fine, but wondering if we need some what to present it 🤔 (although not sure if we want a log.Debug because they're so ugly on the commandline.. maybe?

response, err := loginWithDeviceCodeFlow(ctx, dockerCli)
// if the error represents a failure to initiate the device-code flow,
// then we fallback to regular cli credentials login
if !errors.Is(err, manager.ErrDeviceLoginStartFail) {
return response, err
}
fmt.Fprint(dockerCli.Err(), "Failed to start web-based login - falling back to command line login...\n\n")

I wish the CLI had a location to log to (without that meaning "print and show the user")

@thaJeztah thaJeztah merged commit 667d9fd into docker:27.x Sep 5, 2024
99 checks passed
@thaJeztah thaJeztah deleted the 27.x_backport_mod_tidy branch September 5, 2024 12:53
@laurazard
Copy link
Member

Yeah, we could do something like a log.Debug.

@thaJeztah
Copy link
Member Author

It's a tricky one, due to the nature of the cli being short-lived; we'll be potentially opening a can of worms to handle concurrency, log-rotation, and multiple instances of the cli probably needing something to associate logs with each invocation.

Well, or have a new file created at all times, but we'd still need something to clean it up. Sending to a logging service would be ideal, but not available in all cases.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/refactor PR's that refactor, or clean-up code status/2-code-review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants