Refactored packages

[zen-dog]

Summary:
packages package (!) has been restructured to better separate the underlying concerns. The new structure looks like following:

pkg/kudoctl/packages/
├── reader
│   ├── parser.go
│   ├── read_dir.go
│   ├── reader.go
│   ├── reader_tar.go
│   └── reader_test.go
├── resolver
│   ├── resolver.go
│   ├── resolver_local.go
│   ├── resolver_local_test.go
│   ├── resolver_test.go
│   └── resolver_url.go
├── writer
│   ├── writer_tar.go
│   └── writer_test.go
├── testdata
│   └── ...
├── types.go
└── package.go

tl;dr:

• methods and files have been split across three concerns: reader, writer and resolver
• finder got renamed to resolver: it doesn't so much search for packages as it does resolving passed package name to an underlying location (ulr, local)
• eliminated code duplication e.g. util/kudo/resources.go was reimplementing what resolver is doing
• all the types now live in packages/types.go and are addressed with package prefix e.g. packages.Files. This allowed shortening type names.
• moved other methods to where they were used e.g. new digest.go is living in the repo now
• install.go and upgrade.go commands are now using the resolver directly

[zen-dog]

Fun fact: bad test case was throwing an error, but not for the reason assumed in this test. - oops is simply ignored by the unmarshaller since it is an unknown field, however it has \t in front of it instead of spaces and that's what was causing an error! I removed this test case since all params fields are currently omitempty and hence there is no real bad case to test.

[zen-dog]

Most of the code changes in this PR are non-functional: a lot of code was reshuffled, methods renamed, code duplication eliminated. Biggest functional change:

// before: Package was an interface
type Package interface {
	// transformed server view
	GetCRDs() (*Resources, error)
	// working with local package files
	GetPkgFiles() (*PackageFiles, error)
}

// after: it became a struct
type Package struct {
	// transformed server view
	Resources *Resources
	// working with local package files
	Files *Files
}

Package interface was simplified to being a struct and so its implementations tarPackage and filePackage were not needed anymore. However, the functionality behind it is still there.

[alenkacz]

I think this is almost unreal to review on github so I recommend everyone just opening IDEA and going through the package there. I have some small nits to the internal structure of some files but I have postponed that until we merge this, since this is about the overall structure of the package and I absolutely love that direction 👏

I have just one general suggestion/question. As I understand it, you have package "name" as a string, so you pass it to resolver (which is url, local or repo). What do you think about hiding a reader package into resolvers? What I am thinking is something along these lines:

- resolver
-- file
--- basically contents of reader package
-- url
--- url.go
-- repo
--- repo.go

That way we have three resolvers all packaged under resolver package

[zen-dog]

That way we have three resolvers all packaged under resolver package

Both packages, while working closely together have different purposes IMHO. (1) resolver is more about deciding on package kind and fetching it. (2) reader is about reading contents of the package and parsing its files. (2) is fully valid on its own and is used by e.g. package_verify.go. I like the separation of concerns but I'm open to other reasoning.

[nfnt]

Great and super useful refactoring. This will help solve potential circular package reference issues in further refactoring. Some small suggestions regarding some names here.

[alenkacz]

That way we have three resolvers all packaged under resolver package

Both packages, while working closely together have different purposes IMHO. (1) resolver is more about deciding on package kind and fetching it. (2) reader is about reading contents of the package and parsing its files. (2) is fully valid on its own and is used by e.g. package_verify.go. I like the separation of concerns but I'm open to other reasoning.

I am not sure I agree but maybe I am missing something. I see resolver as the most top-level object. It resolves a package from string, that string can be url, it can be a local file or name in the repo. If it is a local file, it calls reader.Read() so in my mind, the file reader is just one implementation of the resolver. Right now, the localFinder (which I still think should be a local resolver) does not do anything else than call reader.Read so I kind of feel like the reader implementation belongs here (into the local resolver).

It's not a big issue though :) it makes sense to me that way but I won't be blocking this PR on that.

[zen-dog]

Right now, the localResolver (which I still think should be a local resolver) does not do anything else than call reader.Read

ah, I'm starting to see what you mean. The reader.Read method is actually doing the resolving which is better placed in the localResolver. And once we do that, the reader package becomes a few helper methods for the resolver and are better placed there. You convinced me!

[zen-dog]

...however, when trying to do so, it created circular dependencies. Maybe we could improve on it in a followup PR.

[nfnt]

LGTM, but please check if the integration test failure is a new flake or caused by the latest changes.