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

Restructure pack client to pkg package #1315

Merged
merged 15 commits into from
Nov 2, 2021
Merged

Conversation

jromero
Copy link
Member

@jromero jromero commented Nov 1, 2021

Summary

This is a major restructuring of certain packages to make them publically accessible as a go library.

It takes into consideration the proposed structure of RFC#82

Output

Before

.
├── acceptance
├── benchmarks
├── builder
├── buildpackage
├── cmd
│   └── pack
├── config
├── internal
│   ├── blob
│   ├── build
│   ├── builder
│   ├── buildpack
│   ├── buildpackage
│   ├── cache
│   ├── commands
│   ├── config
│   ├── container
│   ├── dist
│   ├── fakes
│   ├── image
│   ├── inspectimage
│   ├── layer
│   ├── logging
│   ├── name
│   ├── paths
│   ├── registry
│   ├── slices
│   ├── sshdialer
│   ├── stack
│   ├── strings
│   ├── stringset
│   ├── style
│   └── termui
├── logging
├── pkg
│   ├── archive
│   └── project
├── registry
├── resources
├── testdata
├── testhelpers
│   └── comparehelpers
├── testmocks
└── tools
    └── pedantic_imports

After

.
├── acceptance
├── benchmarks
├── builder
├── buildpackage
├── cmd
│   └── pack
├── internal
│   ├── build
│   ├── builder
│   ├── cache
│   ├── commands
│   ├── config
│   ├── container
│   ├── fakes
│   ├── inspectimage
│   ├── layer
│   ├── name
│   ├── paths
│   ├── registry
│   ├── slices
│   ├── sshdialer
│   ├── stack
│   ├── strings
│   ├── stringset
│   ├── style
│   ├── term
│   └── termui
├── pkg
│   ├── archive
│   ├── blob
│   ├── buildpack
│   ├── client
│   ├── dist
│   ├── image
│   ├── logging
│   ├── project
│   └── testmocks
├── registry
├── resources
├── testdata
├── testhelpers
│   └── comparehelpers
└── tools
    └── pedantic_imports

Documentation

  • Should this change be documented?
    • Yes, see #___
    • No

Related

Resolves #1245

@jromero jromero added the lib Issue or PR applying only to the use as a library. label Nov 1, 2021
@github-actions github-actions bot added this to the 0.22.0 milestone Nov 1, 2021
@github-actions github-actions bot added type/chore Issue that requests non-user facing changes. type/enhancement Issue that requests a new feature or improvement. labels Nov 1, 2021
Signed-off-by: Javier Romero <[email protected]>
Signed-off-by: Javier Romero <[email protected]>
Before this change, the BuildpackDownloader leaked internal packages
making it unusable as a library.

The primary changes were to move dist and logging package to pkg.

Signed-off-by: Javier Romero <[email protected]>
@jromero jromero added type/bug Issue that reports an unexpected behaviour. and removed type/chore Issue that requests non-user facing changes. type/enhancement Issue that requests a new feature or improvement. labels Nov 1, 2021
We are still using go1.14, io.Discard is available as of go1.16

Signed-off-by: Javier Romero <[email protected]>
@github-actions github-actions bot added type/chore Issue that requests non-user facing changes. type/enhancement Issue that requests a new feature or improvement. labels Nov 1, 2021
@jromero jromero removed type/chore Issue that requests non-user facing changes. type/enhancement Issue that requests a new feature or improvement. labels Nov 1, 2021
@github-actions github-actions bot added type/chore Issue that requests non-user facing changes. type/enhancement Issue that requests a new feature or improvement. labels Nov 1, 2021
@codecov
Copy link

codecov bot commented Nov 2, 2021

Codecov Report

Merging #1315 (7ee83ee) into main (c845240) will decrease coverage by 0.17%.
The diff coverage is 79.39%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1315      +/-   ##
==========================================
- Coverage   80.46%   80.29%   -0.16%     
==========================================
  Files         144      143       -1     
  Lines        9039     8998      -41     
==========================================
- Hits         7272     7224      -48     
- Misses       1301     1307       +6     
- Partials      466      467       +1     
Flag Coverage Δ
os_linux 78.94% <79.39%> (-0.17%) ⬇️
os_macos 73.47% <78.63%> (-0.16%) ⬇️
os_windows 80.16% <79.39%> (-0.13%) ⬇️
unit 80.29% <79.39%> (-0.16%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

@dfreilich
Copy link
Member

One question - what do you think of folding together the internal/logging and pkg/logging package? I'd been planning on making a PR for that purpose, since it becomes non-trivial for a library user to manage the logs (i.e, if you just do pack.NewClient, you automatically get debug level logs, and it's not simple to ensure that you don't get it). That may deserve its own PR, though...

@jromero
Copy link
Member Author

jromero commented Nov 2, 2021

One question - what do you think of folding together the internal/logging and pkg/logging package? I'd been planning on making a PR for that purpose, since it becomes non-trivial for a library user to manage the logs (i.e, if you just do pack.NewClient, you automatically get debug level logs, and it's not simple to ensure that you don't get it). That may deserve its own PR, though...

I did actually do that as part of this PR but primarily because it caused a lot of circular dependencies. The plus side is the better UX as you mentioned.

The tree above had some dangling directories. Updating now...

@jromero jromero marked this pull request as ready for review November 2, 2021 19:02
@jromero jromero requested a review from a team as a code owner November 2, 2021 19:02
@jromero jromero removed type/bug Issue that reports an unexpected behaviour. type/chore Issue that requests non-user facing changes. labels Nov 2, 2021
@jromero jromero merged commit 3e1f522 into main Nov 2, 2021
@jromero jromero deleted the feature/pack-client-lib branch November 2, 2021 19:03
@dfreilich
Copy link
Member

I totally didn't see you merged this – fabulous, thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lib Issue or PR applying only to the use as a library. type/enhancement Issue that requests a new feature or improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Can no longer provide alternative implementations of ImageFetcher
2 participants