provide required schema for basic template

[grokspawn]

Description of the change:
This standardizes all templates on formal schema (#1331), and lays groundwork for standardization of input (#1332) to support #1334 following a roadmap listed in #1330.

IMPORTANT NOTE!

Working on this, it made a lot of sense for opm to own the conversion to a template from FBC, since it controls/supports the templates' definitions. So this PR introduces a new opm alpha convert-template command which explicitly only provides support for the basic template.

Solves #1332

Motivation for the change:
Eliminating the need for users (pipelines, authors, etc.) to be aware of which supported template type is being processed.

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

[codecov]

Codecov Report

Attention: Patch coverage is 0% with 61 lines in your changes missing coverage. Please review.

Project coverage is 48.14%. Comparing base (4c106e2) to head (0ea686b).
Report is 2 commits behind head on master.

Files	Patch %	Lines
alpha/template/basic/basic.go	0.00%	41 Missing ⚠️
alpha/template/converter/converter.go	0.00%	14 Missing ⚠️
alpha/declcfg/load.go	0.00%	6 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1335      +/-   ##
==========================================
- Coverage   48.36%   48.14%   -0.23%     
==========================================
  Files         133      134       +1     
  Lines       12693    12752      +59     
==========================================
  Hits         6139     6139              
- Misses       5516     5575      +59     
  Partials     1038     1038              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[grokspawn]

Sending this to WIP as when I was working on the accompanying docs update it passed my pain threshold for using.

[bentito]

Sorry if this lacks context, or has already been discussed and discarded, but would it make sense to move this, and maybe other generally useful, FBC functions to the O-F API?

[grokspawn]

Once we get some more use/feedback/maturity in the interface, the plan is to eventually promote the catalog templates out of alpha and into pkg.

[everettraven]

Suggested change

	case "yaml":
	fallthrough
	case "json":
	converter.OutputFormat = output
	case "yaml", "json":
	converter.OutputFormat = output

[grokspawn]

Reasoning? When I wrote it I found (for e.g. here) no idiomatic argument for either approach.

[perdasilva]

I was gonna say, if this is possible, that's nice =D I can never remember fallthrough - well not exactly true, diarrhea in German is durchfall. So, I have that mnemonic XD

[everettraven]

IMO using the fallthrough directive:

• increases mental complexity for a reader. They have to first understand what the fallthrough directive does and then build the mental model of "If the yaml case is hit it will fallthrough and do whatever the case underneath of it is, which is the json case and therefore will set the OutputFormat"
• fallthrough means execute the logic of the case underneath and could result in an easier introduction of bugs. What happens if someone adds an additional case below the yaml case that slips through and completely changes the behavior when yaml is specified as the output format?

Instead, using the OR syntax for a switch case is:

• Easier to reason about and build a mental model. Simply looking at the case statement tells me that either the value yaml or json executes the exact same code.
• Less prone to introducing bugs inadvertently as there is a well defined execution path that is not dependent on the ordering of case statements

[grokspawn]

I guess it's a question of subjective contexts here. I infinitely prefer the explicit fallthrough idiom over for e.g. C's implicit (and problematic) fallthrough.
And similarly, I prefer to have a single evaluated condition examined per case, based on experience with other languages.
Like I said... context.

Not a significant factor for me so I made the changes to unblock the review.

(Also, eww Per. 😉)

[everettraven]

Nit: This is an internal function, and I know it was mostly a move, but the implementation only ever looks at the first argument. It seems reasonable to just make this a single string argument instead of a slice of strings

Suggested change

	func OpenFileOrStdin(cmd *cobra.Command, args []string) (io.ReadCloser, string, error) {
	func OpenFileOrStdin(cmd *cobra.Command, path string) (io.ReadCloser, string, error) {

[grokspawn]

This has bugged me too, but it does actually check for empty args as a condition for reading from stdin instead.
We could make it pass just the input path and have each caller test for empty args (and pass in an empty string) but then the function's utility is significantly reduced.

[everettraven]

I don't feel super strongly one way or the other, and saw the empty args check but figured an empty string would work equivalently. This shouldn't block this PR if you'd rather keep it as is.

[everettraven]

Saw a few small things, but nothing that I think is worth holding this up over if you feel they aren't worth addressing

[perdasilva]

lgtm - though I'd also suggest a someone more across the subject to ensure edge cases, etc. don't fall through the cracks.

[openshift-ci]

[APPROVALNOTIFIER] This PR is APPROVED

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

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,perdasilva]

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

[everettraven]

/lgtm