implement `opm alpha render-veneer semver` #948

grokspawn · 2022-04-25T20:21:51Z

NOTE: Updated comments/docs from yesterday's peer review.

Signed-off-by: Jordan Keister jordan@nimblewidget.com

Description of the change:
Implements a "SemVer" Veneer rendering capability, to illustrate the potential benefits of the use of veneer as entry points into the FBC ecosystem.

Since a veneer is identified as an input schema which may be processed to generate a valid FBC, we can define a semver veneer as a schema which uses channel conventions to facilitate the auto-generation of channels along semver delimiters.

Please see the README.md for complete description.

Motivation for the change:
Illustrate a possible effort-saving work flow for operator authors to use to facilitate migration to FBC.

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 · 2022-04-26T20:38:31Z

Codecov Report

Merging #948 (d252a64) into master (0899512) will increase coverage by 0.12%.
The diff coverage is 58.79%.

❗ Current head d252a64 differs from pull request most recent head 6ff7571. Consider uploading reports for the commit 6ff7571 to get more accurate results

@@            Coverage Diff             @@
##           master     #948      +/-   ##
==========================================
+ Coverage   52.48%   52.61%   +0.12%     
==========================================
  Files         103      104       +1     
  Lines        9240     9422     +182     
==========================================
+ Hits         4850     4957     +107     
- Misses       3468     3535      +67     
- Partials      922      930       +8

Impacted Files	Coverage Δ
alpha/veneer/semver/semver.go	`58.79% <58.79%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0899512...6ff7571. Read the comment docs.

Signed-off-by: Jordan Keister <jordan@nimblewidget.com>

grokspawn · 2022-05-11T18:34:31Z

preview comments:

- default to skip patch, with hidden flag for replaces approach
- pull package names from olm.package -> name
- set default-channel in package based on the highest stable > fast > candidate channel available
- limit veneer to one valid bundle name (i.e. bundles of both foo and bar type generate error, while bundles of only foo are OK)
- get versions of bundles from the bundle renders, as olm.bundle -> properties -> olm.package -> version
- channel names should only include significant digits (e.g. major channel stable-v0 contains all 0.. bundles
- minor channels need to include a replace value from the head of the previously-versioned channel (e.g. stable-v1.1.0 should replace stable-v1.0.0 -- without crossing major version boundaries)
- try reverting go.mod to see if we can eliminate go.mod/go.sum/vendor changes from this PR.

later review comments:

- major channels should have replaces steps across minor version transitions

joelanford · 2022-05-11T19:34:42Z

channels need to include a replace value from the head of the previously-versioned channel

this is true

(e.g. stable-v1 should replace stable-v0 -- if defined)

but this isn't. we shouldn't include that replaces value if we're crossing over a major version number transition.

So only minor version channels will have these channel cross-over replaces edges.

joelanford · 2022-05-11T19:37:04Z

should we error out if the necessary channel hasn't been generated because user did not supply any v0 bundles?

No, there should only be edges that connect to actual bundles listed in the veneer. If the veneer contains some v1.1 bundles, but no v1.0 bundles, then the leaf nodes of that v1.1 channels will all be skips and there will be no replace to a previous v1.0 bundle (because there isn't one).

alpha/veneer/semver/semver.go

alpha/veneer/semver/README.md

awgreene

This looks good to me, no major functional changes requested. Great work!

alpha/veneer/semver/README.md

alpha/veneer/semver/semver.go

alpha/veneer/semver/semver_test.go

awgreene · 2022-06-22T22:25:46Z

/lgtm

Signed-off-by: Jordan Keister <jordan@nimblewidget.com> Signed-off-by: Ankita Thomas <ankithom@redhat.com>

awgreene

/lgtm

dinhxuanvu

/approve

openshift-ci · 2022-06-23T18:49:38Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ankitathomas, awgreene, dinhxuanvu, grokspawn

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~~ [dinhxuanvu]

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

tylerslaton

Had some left-over comments after this got merged. Not blocking at all so we can address later if we feel the need.

/lgtm

tylerslaton · 2022-06-23T18:09:12Z

alpha/veneer/semver/semver.go

+// IO structs -- BEGIN
+type semverVeneerBundleEntry struct {
+	Image string `json:"image,omitempty"`
+}
+
+type candidateBundles struct {
+	Bundles []semverVeneerBundleEntry `json:"bundles,omitempty"`
+}
+type fastBundles struct {
+	Bundles []semverVeneerBundleEntry `json:"bundles,omitempty"`
+}
+type stableBundles struct {
+	Bundles []semverVeneerBundleEntry `json:"bundles,omitempty"`
+}
+
+type semverVeneer struct {
+	Schema                string           `json:"schema"`
+	GenerateMajorChannels bool             `json:"generateMajorChannels,omitempty"`
+	GenerateMinorChannels bool             `json:"generateMinorChannels,omitempty"`
+	AvoidSkipPatch        bool             `json:"avoidSkipPatch,omitempty"`
+	Candidate             candidateBundles `json:"candidate,omitempty"`
+	Fast                  fastBundles      `json:"fast,omitempty"`
+	Stable                stableBundles    `json:"stable,omitempty"`
+
+	pkg            string `json:"-"` // the derived package name
+	defaultChannel string `json:"-"` // detected "most stable" channel head
+}
+
+// IO structs -- END


nit: I'd personally like to see an individual comment describing each of these struct but understand that may not be a shared opinion. My thought here is that it makes it easier for maintainers to come through later and understand how these structs are used faster.

tylerslaton · 2022-06-23T18:11:16Z

alpha/veneer/semver/semver.go

+
+// channel "kinds", restricted in this iteration to just these
+const (
+	candidateChannelName string = "candidate"


Can definitely be followed-up since this is an alpha command, but it is relatively trivial to add configurable channels for this so we have both these defaults and consumer-defined ones. Have thought about making a flag that we iterate over for these in a follow-up? The flag type can even be of type map[string]int like you define on line 64 making these feel pretty natural.

The goal of this particular veneer was to focus on and encourage the candidate < fast < stable channel set as a MVP which endorses/encourages our internal practice. "Guidelines and guardrails" is how I like to refer to it.

It wouldn't be hard, but it was counter to our goals at this stage.

We can iterate into a variant or leave it as an exercise for the operator author who needs it. Early feedback will help guide us.

alpha/veneer/semver/semver.go

openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 25, 2022

openshift-ci bot requested review from dinhxuanvu and kevinrizza April 25, 2022 20:22

grokspawn changed the title ~~[WIP] implement opm alpha render-veneer semver, prior art from https://github.com/joelan…~~ [WIP] implement opm alpha render-veneer semver Apr 25, 2022

grokspawn requested a review from joelanford April 26, 2022 21:13

grokspawn force-pushed the grokspawn-semver-veneer-cmd branch from 9e66aeb to aa860cd Compare May 9, 2022 12:51

openshift-ci bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 9, 2022

grokspawn force-pushed the grokspawn-semver-veneer-cmd branch from aa860cd to eaa241a Compare May 9, 2022 13:31

openshift-ci bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 9, 2022

grokspawn added a commit that referenced this pull request May 10, 2022

Update history for review. Details at #948 (comment)

f1500b5

Signed-off-by: Jordan Keister <jordan@nimblewidget.com>

grokspawn force-pushed the grokspawn-semver-veneer-cmd branch from c98281d to f1500b5 Compare May 10, 2022 13:14

grokspawn requested review from tylerslaton, timflannagan, exdx, ankitathomas, nsapse and awgreene May 10, 2022 13:28

grokspawn changed the title ~~[WIP] implement opm alpha render-veneer semver~~ implement opm alpha render-veneer semver May 10, 2022

openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 10, 2022

grokspawn requested review from njhale and anik120 May 10, 2022 13:34

openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 11, 2022

grokspawn changed the title ~~implement opm alpha render-veneer semver~~ [WIP] implement opm alpha render-veneer semver May 11, 2022

openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 11, 2022

grokspawn force-pushed the grokspawn-semver-veneer-cmd branch 2 times, most recently from 14c98db to 063997d Compare May 11, 2022 19:13

grokspawn force-pushed the grokspawn-semver-veneer-cmd branch 4 times, most recently from 93bf7e8 to 26f502b Compare June 17, 2022 21:15

grokspawn commented Jun 21, 2022

View reviewed changes

alpha/veneer/semver/semver.go Outdated Show resolved Hide resolved

joelanford reviewed Jun 21, 2022

View reviewed changes

alpha/veneer/semver/semver.go Show resolved Hide resolved

grokspawn commented Jun 21, 2022

View reviewed changes

alpha/veneer/semver/semver.go Show resolved Hide resolved

awgreene reviewed Jun 21, 2022

View reviewed changes

alpha/veneer/semver/README.md Outdated Show resolved Hide resolved

grokspawn force-pushed the grokspawn-semver-veneer-cmd branch from 26f502b to 3962fc8 Compare June 22, 2022 17:17

awgreene requested changes Jun 22, 2022

View reviewed changes

grokspawn force-pushed the grokspawn-semver-veneer-cmd branch 2 times, most recently from cfd56bb to 6569f5d Compare June 22, 2022 21:53

grokspawn requested a review from awgreene June 22, 2022 22:00

awgreene mentioned this pull request Jun 22, 2022

Feature Request: Create a generic writer for veneers #973

Closed

openshift-ci bot assigned awgreene Jun 22, 2022

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jun 22, 2022

- implement opm alpha render-veneer semver

6ff7571

Signed-off-by: Jordan Keister <jordan@nimblewidget.com> Signed-off-by: Ankita Thomas <ankithom@redhat.com>

grokspawn force-pushed the grokspawn-semver-veneer-cmd branch from 6569f5d to 6ff7571 Compare June 23, 2022 14:20

openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Jun 23, 2022

awgreene approved these changes Jun 23, 2022

View reviewed changes

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jun 23, 2022

dinhxuanvu approved these changes Jun 23, 2022

View reviewed changes

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 23, 2022

grokspawn merged commit 1e6c7a7 into master Jun 23, 2022

tylerslaton reviewed Jun 23, 2022

View reviewed changes

grokspawn deleted the grokspawn-semver-veneer-cmd branch June 23, 2022 18:57

grokspawn mentioned this pull request Jul 8, 2022

veneer documentation (alpha) operator-framework/olm-docs#245

Merged

joelanford mentioned this pull request Aug 9, 2022

Question on Veneers #1010

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

implement `opm alpha render-veneer semver` #948

implement `opm alpha render-veneer semver` #948

grokspawn commented Apr 25, 2022 •

edited

codecov bot commented Apr 26, 2022 •

edited

grokspawn commented May 11, 2022 •

edited

joelanford commented May 11, 2022

joelanford commented May 11, 2022 •

edited

awgreene left a comment

awgreene commented Jun 22, 2022

awgreene left a comment

dinhxuanvu left a comment

openshift-ci bot commented Jun 23, 2022

tylerslaton left a comment

tylerslaton Jun 23, 2022

tylerslaton Jun 23, 2022

grokspawn Jun 23, 2022 •

edited

implement opm alpha render-veneer semver #948

implement opm alpha render-veneer semver #948

Conversation

grokspawn commented Apr 25, 2022 • edited

codecov bot commented Apr 26, 2022 • edited

Codecov Report

grokspawn commented May 11, 2022 • edited

joelanford commented May 11, 2022

joelanford commented May 11, 2022 • edited

awgreene left a comment

Choose a reason for hiding this comment

awgreene commented Jun 22, 2022

awgreene left a comment

Choose a reason for hiding this comment

dinhxuanvu left a comment

Choose a reason for hiding this comment

openshift-ci bot commented Jun 23, 2022

tylerslaton left a comment

Choose a reason for hiding this comment

tylerslaton Jun 23, 2022

Choose a reason for hiding this comment

tylerslaton Jun 23, 2022

Choose a reason for hiding this comment

grokspawn Jun 23, 2022 • edited

Choose a reason for hiding this comment

implement `opm alpha render-veneer semver` #948

implement `opm alpha render-veneer semver` #948

grokspawn commented Apr 25, 2022 •

edited

codecov bot commented Apr 26, 2022 •

edited

grokspawn commented May 11, 2022 •

edited

joelanford commented May 11, 2022 •

edited

grokspawn Jun 23, 2022 •

edited