add optional schema migrations; default to olm.bundle.object instead of olm.csv.metadata

[grokspawn]

Description of the change:
Introduces the concept of optional schema migrations to opm available to the render and migrate actions, and template expansions (basic and semver). This includes a sequencing and extension mechanism since we presume that successive schema migrations might be dependent on the successful execution of prior ones.

Schema migrations represent a level, are explicitly requested by name, and the sequence of all migrations up to and including the named one will be executed in sequence.

If any schema migration fails, it is immediately terminal to the sequence.

The existing --migrate flag for opm render is equivalent to specifying the last possible --migrate-level:

./bin/opm render --help
Generate a stream of file-based catalog objects to stdout from the provided
catalog images, file-based catalog directories, bundle images, and sqlite
database files.

...

      --migrate                Perform all available schema migrations on the rendered FBC

...

Information on available migrations can be obtained from the help for associated migrate/render/semver/basic commands, for e.g.:

./bin/opm render --help
Generate a stream of file-based catalog objects to stdout from the provided
catalog images, file-based catalog directories, bundle images, and sqlite
database files.

...
      --migrate-level string   Name of the last migration to run (default: none)

                               The migrator will run all migrations up to and including the selected level.

                               Available migrators:
                                 - none                          : do nothing
                                 - bundle-object-to-csv-metadata : migrates bundles' "olm.bundle.object" to "olm.csv.metadata"
...

Motivation for the change:
When we introduced the new bundle metadata type olm.csv.metadata and made it the default, we started hearing of authors having to select non-latest versions of opm in order to preserve their pipelines' functionality. This is an anti-pattern, since we want to ensure that authors can safely use versions of opm that have the most fixes.

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

Attention: Patch coverage is 56.16438% with 32 lines in your changes missing coverage. Please review.

Project coverage is 48.14%. Comparing base (cac5f2d) to head (3997726).
Report is 24 commits behind head on master.

Files	Patch %	Lines
alpha/action/migrations/migrations.go	48.71%	19 Missing and 1 partial ⚠️
...on/migrations/000_bundle_object_to_csv_metadata.go	61.90%	5 Missing and 3 partials ⚠️
alpha/action/render.go	66.66%	1 Missing and 1 partial ⚠️
alpha/template/basic/basic.go	0.00%	1 Missing ⚠️
alpha/template/semver/semver.go	0.00%	1 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #1384   +/-   ##
=======================================
  Coverage   48.14%   48.14%           
=======================================
  Files         134      136    +2     
  Lines       12752    12780   +28     
=======================================
+ Hits         6139     6153   +14     
- Misses       5575     5593   +18     
+ Partials     1038     1034    -4     

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

[joelanford]

The existing --migrate flag for opm render is deprecated, and will exit with an error if used

That sounds like a breaking change. We could make --migrate and --migrate-level mutually exclusive. If we did that, I think we would need to decide what --migrate means. I can think of two options:

1. It means the exact same thing as --migrate-level=bundle-object-to-csv-metadata (because that's what it means right now).
2. It means "do all of the available migrations". That's what I had imagined it would mean when we introduced it.

[grokspawn]

The existing --migrate flag for opm render is deprecated, and will exit with an error if used

That sounds like a breaking change. We could make --migrate and --migrate-level mutually exclusive. If we did that, I think we would need to decide what --migrate means. I can think of two options:

1. It means the exact same thing as `--migrate-level=bundle-object-to-csv-metadata` (because that's what it means right now).

2. It means "do all of the available migrations". That's what I had imagined it would mean when we introduced it.

This is a breaking change as it is, and I tried to be really up-front to it. I've reversed that now so we can reason about it. I wanted something more elegant for the default-runs-all case than this. There's also the argument to be made that if using the deprecation flag is an easy way to 'select all' then it would be the most common interaction users will have, and why would we want to deprecate it (versus providing overlapping options).

Separately, I'm uncomfortable with the reliance on bare strings instead of typed data for the migration levels, and I was going to spend some more time with it, but in all honesty that can be follow-on work after this merges.

Edit: we elected to

1. allow use of --migrate flag to mean 'all migrations'
2. make use of --migrate and --migrate-level mutually exclusive
   I updated the PR description to match.

[grokspawn]

NB: I haven't updated the PR description based on the recent decision to reverse what the migrate flag means to render, since I think we're on the same page here but not 100% certain. I'll update that with the ultimate result of the review.

[grokspawn]

/hold
still working on the migrations_test in alpha/action/migrations. Just pushed up to share a reference.

[grokspawn]

/hold cancel
unit tests up to date.

[joelanford]

I very well could be misunderstanding this code, but this will fail if migrationPre is ever non-zero, right?

And the only reason it doesn't fail now is because there's only one spec that increments the counter?

I'm having trouble conjuring an approach that remains simple and overcomes this problem, but I'll clone and tinker later.

[grokspawn]

Yes, this does an explicit compare against the counter value in the test spec... which will only work when the count of TCs attempting this feat is one.
It should be verifying that the count went up by the amount in the test spec, instead:

require.Equal(t, migrationCounter, migrationPre+s.migrationCount)

[joelanford]

WDYT about adding input and expectedOutput struct fields here instead of the evaluators indirection?

If we did that, the test code would turn into something like:

assert.NoError(t, migators.Migrate(t.input))
assert.Empty(t, cmp.Diff(t.input, t.expectedOutput))

[grokspawn]

evaluators solves the problem of validating the sequence of migrators indicated by a migration label, whereas a test case may invoke multiple migrators (since the migrators label indicates a sequence [1..n] rather than a single value). It's 1:1 with the available migrators so we can attest to no side effects as we progress through a migrator chain, not 1:1 test-spec.
i.e.
input --> none(unmutated) --> csvMetadata(mutated) --> futureMigration(mutatedagain).

[openshift-ci]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: grokspawn, joelanford

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

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