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

Support Warning header aggregation and reporting in crane #1604

Merged
merged 5 commits into from
Mar 24, 2023

Conversation

imjasonh
Copy link
Collaborator

@imjasonh imjasonh commented Mar 18, 2023

opencontainers/distribution-spec#393 seems to be on a good path, so I figured I'd put in a better implementation of warning collection/reporting in ggcr.

  • adds an internal transport that collects and reports warnings from registry requests
  • adds registry.WithWarning to pkg/registry that probabilistically reports the given warning (WithWarning(0.1, "hi") warns "hi" 10% of the time)

This also updates cmd/registry to add demo warnings some % of the time; I'll probably revert this before merging, this is just for demo purposes. Speaking of demo:

$ go run ./cmd/registry &
$ go run ./cmd/crane cp alpine localhost:1338/alpine
...
[WARNING]: 60% of the time, it works every time.
[WARNING]: This registry is cool.

Logic for parsing the warning message out of the header is pretty janky, and could use some tests.

Clients that use go-containerregistry won't suddenly start reporting warnings without changes. They'll need to inject a transport like crane does -- if we think crane's approach is generally useful we can make it available in pkg/crane itself.

@codecov-commenter
Copy link

codecov-commenter commented Mar 18, 2023

Codecov Report

Merging #1604 (2900894) into main (0f2db49) will decrease coverage by 0.07%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##             main    #1604      +/-   ##
==========================================
- Coverage   72.80%   72.74%   -0.07%     
==========================================
  Files         118      118              
  Lines        9440     9452      +12     
==========================================
+ Hits         6873     6876       +3     
- Misses       1869     1878       +9     
  Partials      698      698              
Impacted Files Coverage Δ
pkg/registry/registry.go 83.33% <0.00%> (-16.67%) ⬇️
pkg/v1/remote/options.go 77.77% <ø> (ø)

... and 1 file with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@imjasonh imjasonh changed the title Support Warning header aggregation and reporting Support Warning header aggregation and reporting in crane Mar 18, 2023
cmd/crane/cmd/root.go Outdated Show resolved Hide resolved
@jonjohnsonjr
Copy link
Collaborator

I think there are a couple ways we could go with this that will be slightly better than adding WithWarnCollector to crane and remote.

We already have logs.Warn (used here in this PR currently). We could just have warnings be written to logs.Warn where we set logs.Warn to do warncollectory things.

There's also remote.WithProgress that we currently use to emit very specific kinds of updates. We could do something similar (or even use the same option) to send clients more kinds of events, e.g. these Warnings.

I really dislike the logs package -- it would probably be better if we were just emitting events and having consumers do things with them...

There's also the slog stuff that has its own Warn level, which maybe we could replace the logs package with 🤔

How do you feel about this not being a public API, and instead we have crane pass in a custom transport that does all these warncollectory things until we have a happier public interface?

cmd/crane/cmd/root.go Outdated Show resolved Hide resolved
@imjasonh
Copy link
Collaborator Author

How do you feel about this not being a public API, and instead we have crane pass in a custom transport that does all these warncollectory things until we have a happier public interface?

I'm fine with that. I'll get something along these lines up shortly.

@imjasonh imjasonh enabled auto-merge (squash) March 24, 2023 17:40
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

4 participants