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

Straighten out remote.List{WithContext} #1090

Merged
merged 1 commit into from
Jul 30, 2021
Merged

Conversation

imjasonh
Copy link
Collaborator

@imjasonh imjasonh commented Jul 28, 2021

ListWithContext is kinda gross when we also have List(..., remote.WithContext(ctx) -- let's deprecate it.

edit: the rest is no longer relevant, but useful for context:
This led a test to fail because the /v2/ request fails on a cancelled context, which I think is good because we shouldn't ignore the provided context for that ping request. In fixing that, I wanted to make the test check for whether the error errors.Is(context.Canceled), but that was stymied by transport.NewTransportWithContext losing that information when it does it's little multi-error thing.

    list_test.go:115: error was not context.Canceled, got Get "https://127.0.0.1:60957/v2/": context canceled; Get "http://127.0.0.1:60957/v2/": context canceled

Gross.

So we have a couple options:

  1. change the test to just search the error message for the string "context canceled" and keep the http fallback multierror logic from before (probably safest)
  2. don't return multiple errors, maybe if logs.Enabled(logs.Debug) { logs.Debug.Printf("failed https, falling back to http") }
  3. drop this whole thing, ListWithContext is fine the way it is, and ignoring context to do the ping is fine.

pkg/v1/remote/list.go Outdated Show resolved Hide resolved
remote.ListWithContext now just wraps remote.List, instead of the other
wawy around. This also has the effect that ping() uses the given
context.

remote.ListWithContext is marked as deprecated, to be removed in a
future update.
@imjasonh imjasonh changed the title WIP: smooth out ListWithContext Straighten out remote.List{WithContext} Jul 29, 2021
@imjasonh imjasonh merged commit a3a06bb into google:main Jul 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants