refactor to move more processing to the API

[grokspawn]

Description of the change:
move all parsing and template logic into the API, out of the cobra command parser
facilitate testing of the HTTP access for catalog files

to accomplish this, we exposed a dynamic set of builders as a property of the template, but we didn't want to export it, so that meant unexporting all template fields and leveraging the options pattern to provide access to external callers. This meant that we could mock out the builder interactions with dummy objects to limit test interactions to just this package.
In addition, we moved the http/filesystem processing for the catalogs.yaml access to an exported function so we could utest interactions here.

Motivation for the change:
to facilitate ability to test with different input file/URI

Reviewer Checklist

• Implementation matches the proposed design, or proposal is updated to match implementation
• Sufficient unit test coverage
• Sufficient end-to-end test coverage
• Docs updated or added to /docs
• Commit messages sensible and descriptive

[openshift-ci]

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

[codecov]

Codecov Report

Merging #1082 (4a5ad8a) into master (49eae7b) will increase coverage by 0.37%.
The diff coverage is 88.39%.

❗ Current head 4a5ad8a differs from pull request most recent head 097ed19. Consider uploading reports for the commit 097ed19 to get more accurate results

@@            Coverage Diff             @@
##           master    #1082      +/-   ##
==========================================
+ Coverage   52.57%   52.95%   +0.37%     
==========================================
  Files         107      107              
  Lines        9384     9482      +98     
==========================================
+ Hits         4934     5021      +87     
- Misses       3523     3531       +8     
- Partials      927      930       +3     

Impacted Files	Coverage Δ
alpha/template/composite/composite.go	89.09% <87.50%> (-10.91%)	⬇️
alpha/template/composite/builder.go	83.44% <90.90%> (-0.12%)	⬇️
pkg/cache/json.go	53.15% <100.00%> (+1.75%)	⬆️
pkg/cache/pkgs.go	70.63% <100.00%> (+1.03%)	⬆️

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

[everettraven]

Overall looks good. I have a concern about using cmd.CombinedOutput vs cmd.Output and a few other nits/comments

[everettraven]

Nit: I get what we are going for here, but I think using an if here ends up being more explicit about the intentions and makes it a bit more readable

Suggested change

	switch output {
	case "yaml":
	// do nothing
	case "json":
	// do nothing
	default:
	log.Fatalf("invalid --output value %q, expected (json|yaml)", output)
	if output != "yaml" && output != "json" {
	log.Fatalf("invalid --output value %q, expected (json|yaml)", output)

[oceanc80]

+1
Unless we think we'll add customizations here for each case, I'd agree that it's a bit clearer using an if statement.

[grokspawn]

I'll push back on this one, because I noticed it too but made the conscious choice to perpetuate a common pattern which was also demonstrated in
cmd/opm/init/cmd.go
cmd/opm/render/cmd.go
cmd/opm/alpha/template/basic.go

[oceanc80]

Right, but in all three of those cases there is custom logic for the yaml and json cases vs a no-op.
e.g. https://github.com/operator-framework/operator-registry/blob/master/cmd/opm/alpha/template/basic.go#L41-L48

[everettraven]

+1 to @oceanc80's point

[everettraven]

Not a huge deal though, so if you prefer leaving this as is I'm okay with it

[everettraven]

Looks good to me. Have a comment that could be addressed now or in a follow up - I'll leave it up to you.

Going to hold off on adding the LGTM label to prevent an auto-merge so others have a chance to review

[everettraven]

Could be done in a follow up, but is the .Destination.BaseImage field still necessary since we aren't running opm images anymore? If the image isn't going to be taken into account by the rendering process we should probably remove it (or at least comment it out if we plan to use it later) to prevent confusion

[grokspawn]

I agree that this should be commented-out, since I don't think discussion is dead on it right now, but we definitely don't want to enforce it.
However, because dependencies are starting to stack up against this PR refactor, I'd prefer to handle in another PR.

[oceanc80]

lgtm

[openshift-ci]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: everettraven, grokspawn, oceanc80

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [grokspawn,oceanc80]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[everettraven]

/lgtm