Towards eliminating v1util

[mattmoor]

This blows apart v1util into smaller (mostly internal) packages, keeping only aliases of what we have today under pkg/v1/v1util to avoid breaking downstreams, but adding a deprecation notice, which should trigger golangci-lint to start flagging users of the deprecated methods.

[jonjohnsonjr]

Drop the stutter?

[mattmoor]

What do you want to call the Gunzip variant :)

[jonjohnsonjr]

You got me there.

[mattmoor]

I was piggybacking on the sentiment @imjasonh had below that renaming things becomes reeeeaaaaal easy under internal/, so this is trivial to fixup later if we find a name we like. 🤷

[jonjohnsonjr]

I think the convention is Deprecated: per https://github.com/golang/go/wiki/Deprecated

[imjasonh]

pkg/v1/v1util -> pkg/v1/internal has roughly the same effect, and is simply mechanical. Splitting it into separate internal packages and moving some to still-exported symbols in v1 seems like more work than necessary.

[mattmoor]

pkg/v1/v1util -> pkg/v1/internal has roughly the same effect, and is simply mechanical

Are you suggesting we have a literal internal package? I kinda thought part of moving away from v1util was to get better package naming.

and moving some to still-exported symbols in v1 seems like more work than necessary

Not parsing, but exposing the things that were previously public under v1util with Deprecated: was intentional to not break folks immediately.

[jonjohnsonjr]

Not parsing, but exposing the things that were previously public under v1util with Deprecated: was intentional to not break folks immediately.

+1, we owe them one release of deprecated before we drop it, IMO.

[mattmoor]

I can TODO(#1234) removing, if we want to track this better.

[imjasonh]

Are you suggesting we have a literal internal package? I kinda thought part of moving away from v1util was to get better package naming.

I'm suggesting we stage the change first by making it internal (to shake off external deps), then we have the flexibility to move methods as unexported where they're used if we want.

Package naming and organization matters a lot less to me inside internal where it's clear it's only for our own uses.

Not parsing, but exposing the things that were previously public under v1util with Deprecated: was intentional to not break folks immediately.

Yeah, that's fine, I was talking about v1.VerifyReadCloser, which I think we should keep internal unless we know somewhere important someone's depending on it.

[mattmoor]

I'm suggesting we stage the change first by making it internal (to shake off external deps)

Just making it internal is breaking, and I don't think we should just break folks currently using it (until a few days ago this included ko, are we sure Bazel isn't using this?).

The intent of staging things this way was to make it clear that the external exposure is Deprecated (and will break), while simultaneously enabling us to restructure things in a way that's consistent with what y'all are asking for with pkg/v1/internal/estargz.

Package naming and organization matters a lot less to me inside internal where it's clear it's only for our own uses

I completely agree, which is why I didn't sweat the "stutter" stuff right away.

I was talking about v1.VerifyReadCloser

I'm happy to make this private, I don't really care, but digest verification just felt like something folks would want, but it is not a hill I'd die on 😉

[mattmoor]

Alright, we now have verify.ReadCloser in an internal package (aliased as deprecated v1util.VerifyReadCloser)

[imjasonh]

To be clear I'm absolutely on board with aliasing + deprecating for now. It was the atomizing into internal packages I was commenting about. Sorry that wasn't clear.

[mattmoor]

It feels like we are quibbling about internal structure, which we all agreed that we can now change whenever we want. I went with this because it seemed like it was consistent with what was being suggested for estargz, and it seemed like we all agreed about this on slack:

The "consistent structure" I was talking about above was "the atomizing" 😅

Perhaps we're talking past each other, so it may be simplest to hop on a zoom (🤔 now I'm wondering if this is a ploy to see the 🐶 , very :sus:)

[codecov-io]

Codecov Report

Merging #872 (245630d) into master (8b5370a) will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master     #872   +/-   ##
=======================================
  Coverage   74.28%   74.28%           
=======================================
  Files         106      106           
  Lines        4476     4476           
=======================================
  Hits         3325     3325           
  Misses        656      656           
  Partials      495      495           

Impacted Files	Coverage Δ
pkg/v1/internal/and/and_closer.go	100.00% <ø> (ø)
pkg/v1/internal/gzip/zip.go	100.00% <100.00%> (ø)
pkg/v1/internal/verify/verify.go	100.00% <100.00%> (ø)
pkg/v1/mutate/mutate.go	71.97% <100.00%> (ø)
pkg/v1/partial/compressed.go	73.68% <100.00%> (ø)
pkg/v1/partial/uncompressed.go	73.52% <100.00%> (ø)
pkg/v1/remote/descriptor.go	73.78% <100.00%> (ø)
pkg/v1/remote/image.go	79.45% <100.00%> (ø)
pkg/v1/tarball/image.go	77.77% <100.00%> (ø)
pkg/v1/tarball/layer.go	71.83% <100.00%> (ø)
... and 2 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8b5370a...245630d. Read the comment docs.

[mattmoor]

@imjasonh I think this covers the key points that we discussed. I am happy to keep making changes, which should be easy now that this stuff is under internal/. Let me know if you want to chat, and I can /zoom you.

[imjasonh]

Lgtm, thanks for putting up with my quibbling 😆