OCPBUGS-1272: Add pipe support to "opm alpha render-veneer basic" #1026

oceanc80 · 2022-09-27T23:15:18Z

Add pipe support to the render-veneer basic command and expand basic veneer validation checks.

Signed-off-by: Catherine Chan-Tse cchantse@redhat.com

Description of the change:

Add pipe support to "opm alpha render-veneer basic"
Expand basic veneer validation checks and fail when non-veneer bundles are passed in

Motivation for the change:

Addresses OCPBUGS-1272

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-09-27T23:22:38Z

Codecov Report

Merging #1026 (de60ef8) into master (0e53caf) will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #1026   +/-   ##
=======================================
  Coverage   51.95%   51.95%           
=======================================
  Files         102      102           
  Lines        9215     9215           
=======================================
  Hits         4788     4788           
  Misses       3514     3514           
  Partials      913      913

Impacted Files	Coverage Δ
alpha/declcfg/load.go	`78.65% <100.00%> (ø)`
alpha/model/model.go	`92.64% <0.00%> (ø)`
pkg/registry/query.go	`61.48% <0.00%> (ø)`
alpha/property/property.go	`92.68% <0.00%> (ø)`
pkg/lib/indexer/interfaces.go	`0.00% <0.00%> (ø)`

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

cmd/opm/alpha/veneer/basic.go

alpha/declcfg/load.go

grokspawn · 2022-09-28T13:40:22Z

alpha/veneer/basic/basic.go

 		if !isBundleVeneer(&b) {
-			outb = append(outb, b)
-			continue
+			return nil, fmt.Errorf("bundle veneers must define only a schema and an image; all other fields must not be defined")
 		}


The original intent of this code was to allow the author to provide a combination of completely-rendered olm.bundle references as well as 'shorthand-ed' ones which contained only the image references.
If a bundle contained properties, it was presumed to be of the completely-rendered variety and just accumulated (i.e. the author was responsible for the completeness of the rendering).
If a bundle was without properties, it was presumed to be a shorthand bundle, and we would render it ourselves and accumulate it to the output.

I don't love the idea of refusing to accept any completely-rendered bundles in the veneer, but there is some question of why any author would choose to mix-and-match in that fashion anyway.

My perspective here is to make things as strict and locked down as possible at first so that we can attempt to keep the scope of what we need to support small. We can always open things up to support that use case in the future, but it we support it now, we're signing up for it now and we might not understand the ways that people might use/abuse it.

grokspawn · 2022-09-28T13:57:53Z

alpha/declcfg/load.go

 }

-func readYAMLOrJSON(r io.Reader) (*DeclarativeConfig, error) {
+func LoadReader(r io.Reader) (*DeclarativeConfig, error) {


What was the benefit of the rename? I may be missing some background but readYAMLOrJSON, while verbose, does tell us what it does while LoadReader is more vague.

Reading between the lines ... is it because this interface was not exported?

I guess the reason I'd lean towards reader vs yaml or json is just because I feel like I'd be more likely to assume that readYAMLOrJSON would take yaml or json as an input vs a reader, but I'm also not super wedded to it.

IMO, its because we're going to export it, and since we're exporting it, we're following the pattern of the other exported functions that return (declcfg.DeclarativeConfig, error)

grokspawn · 2022-09-28T13:58:51Z

alpha/veneer/basic/basic.go

-
-	cfg, err := declcfg.LoadFile(root, fname)
+func (v Veneer) Render(ctx context.Context, reader io.Reader) (*declcfg.DeclarativeConfig, error) {
+	cfg, err := declcfg.LoadReader(reader)


We were originally using declcfg.LoadFile here because we wanted the follow-on call to readBundleObjects. Do we no longer want that?

LoadFile doesn't work with an arbitrary io.Reader because readBundleObjects needs to have some awareness of the directory in which the file containing the bundle object was found. That way it can de-reference those bundle object paths, using that directory as the root.

The root of the problem is essentially that the basic veneer input is not the same as FBC, but we're using FBC loaders because the basic veneer input is "close" to FBC.

joelanford · 2022-09-28T20:15:10Z

alpha/veneer/basic/basic.go

 		if !isBundleVeneer(&b) {
-			outb = append(outb, b)
-			continue
+			return nil, fmt.Errorf("bundle veneers must define only a schema and an image; all other fields must not be defined")


I think I would just say something more generic, something like: "unexpected fields present in basic veneer bundle"

"all other fields must not be defined" gives the impression that there are other fields that should be left empty. In my mind, that's probably the wrong message. Basic veneer is its own format that just happens to look a bit like FBC.

I say this because it gives us more flexibility in the future if we define the basic veneer input file separately from FBC. The two formats don't necessarily have to evolve in lock-step.

I lean heavily toward getting the basic veneer on its own schema, so we could do opm alpha render-veneer instead of opm alpha render-veneer [basic|semver].

joelanford · 2022-09-28T20:19:07Z

cmd/opm/alpha/veneer/basic.go

-		Long:  `Generate a declarative config blob from a single 'basic veneer' file, typified as a declarative configuration file where olm.bundle objects have no properties`,
-		Args:  cobra.ExactArgs(1),
+		Short: "Generate a declarative config blob from a single 'basic veneer' file \nWhen FILE is '-' or not provided, the veneer is read from standard input",
+		Long:  "Generate a declarative config blob from a single 'basic veneer' file, typified as a declarative configuration file where olm.bundle objects have no properties \nWhen FILE is '-' or not provided, the veneer is read from standard input",


Two in one suggestion:

You can use backticks and actual newlines in the code (rather than escape newline characters), which can help with readability.

Drop the mention of FBC, minus bundle properties. re, my previous comment.

Suggested change

Long: "Generate a declarative config blob from a single 'basic veneer' file, typified as a declarative configuration file where olm.bundle objects have no properties \nWhen FILE is '-' or not provided, the veneer is read from standard input",

Long: `Generate a declarative config blob from a single 'basic veneer' file.

When FILE is '-' or not provided, the veneer is read from standard input.`,

joelanford · 2022-09-28T20:21:58Z

cmd/opm/alpha/veneer/basic.go

+			data, source, err := OpenFileOrReadStdin(cmd, args)
+			if err != nil {
+				log.Fatalf("unable to read %q: %v", source, err)


couple of nits here:

we're not reading stdin yet, we're just opening it for reading.

do we need OpenFileOrReadStdin to be consumed outside this package? If not, make it unexported.

Suggested change

data, source, err := OpenFileOrReadStdin(cmd, args)

if err != nil {

log.Fatalf("unable to read %q: %v", source, err)

data, source, err := OpenFileOrStdin(cmd, args)

if err != nil {

log.Fatalf("unable to open %q: %v", source, err)

cmd/opm/alpha/veneer/cmd.go

cmd/opm/alpha/veneer/basic.go

grokspawn · 2022-09-29T14:01:11Z

cmd/opm/alpha/veneer/cmd.go

+func openFileOrStdin(cmd *cobra.Command, args []string) (io.ReadCloser, string, error) {
+	if len(args) == 0 || args[0] == "-" {
+		return io.NopCloser(cmd.InOrStdin()), "stdin", nil
+	}
+	reader, err := os.Open(args[0])
+	return reader, args[0], err
+}


yay, re-use!

Signed-off-by: Catherine Chan-Tse <cchantse@redhat.com>

ankitathomas

/lgtm

ankitathomas · 2022-09-30T21:02:01Z

alpha/veneer/basic/basic.go

+// but no Properties, RelatedImages or Package defined
 func isBundleVeneer(b *declcfg.Bundle) bool {
-	return len(b.Properties) == 0
+	return b.Schema != "" && b.Image != "" && b.Package == "" && len(b.Properties) == 0 && len(b.RelatedImages) == 0


We should have the field restrictions for the simple veneer documented, specifically the hard requirement for prohibited fields. The error message isn't very helpful here.

openshift-ci · 2022-09-30T21:02:23Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ankitathomas, 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]

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

openshift-ci-robot · 2022-10-04T16:15:19Z

@oceanc80: Jira Issue OCPBUGS-1272 is in an unrecognized state (ON_QA) and will not be moved to the MODIFIED state.

In response to this:

Add pipe support to the render-veneer basic command and expand basic veneer validation checks.

Signed-off-by: Catherine Chan-Tse cchantse@redhat.com

Description of the change:

Add pipe support to "opm alpha render-veneer basic"

Expand basic veneer validation checks and fail when non-veneer bundles are passed in

Motivation for the change:

Addresses OCPBUGS-1272

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

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

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 Sep 27, 2022

openshift-ci bot requested review from exdx and jmrodri September 27, 2022 23:15

joelanford reviewed Sep 28, 2022

View reviewed changes

cmd/opm/alpha/veneer/basic.go Outdated Show resolved Hide resolved

alpha/declcfg/load.go Show resolved Hide resolved

grokspawn reviewed Sep 28, 2022

View reviewed changes

oceanc80 force-pushed the OCPBUGS-1272 branch from d5e0a02 to a0188f4 Compare September 28, 2022 20:09

joelanford reviewed Sep 28, 2022

View reviewed changes

cmd/opm/alpha/veneer/cmd.go Outdated Show resolved Hide resolved

oceanc80 force-pushed the OCPBUGS-1272 branch from a0188f4 to 2cb7213 Compare September 29, 2022 12:57

grokspawn reviewed Sep 29, 2022

View reviewed changes

oceanc80 force-pushed the OCPBUGS-1272 branch from 2cb7213 to b11b214 Compare September 30, 2022 17:59

grokspawn approved these changes Sep 30, 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 Sep 30, 2022

Add pipe support to render-veneer basic command

de60ef8

Signed-off-by: Catherine Chan-Tse <cchantse@redhat.com>

oceanc80 force-pushed the OCPBUGS-1272 branch from b11b214 to de60ef8 Compare September 30, 2022 19:46

oceanc80 changed the title ~~WIP: OCPBUGS-1272 Add pipe support to "opm alpha render-veneer basic"~~ OCPBUGS-1272 Add pipe support to "opm alpha render-veneer basic" Sep 30, 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 Sep 30, 2022

ankitathomas approved these changes Sep 30, 2022

View reviewed changes

openshift-ci bot assigned ankitathomas Sep 30, 2022

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

openshift-merge-robot merged commit 614d6a9 into operator-framework:master Sep 30, 2022

oceanc80 deleted the OCPBUGS-1272 branch October 3, 2022 12:07

grokspawn changed the title ~~OCPBUGS-1272 Add pipe support to "opm alpha render-veneer basic"~~ OCPBUGS-1272: Add pipe support to "opm alpha render-veneer basic" Oct 4, 2022

openshift-ci-robot mentioned this pull request Nov 16, 2022

OCPBUGS-1272: Add pipe support to render-veneer basic command (#1026) openshift/operator-framework-olm#401

Merged

-		Long:  "Generate a declarative config blob from a single 'basic veneer' file, typified as a declarative configuration file where olm.bundle objects have no properties \nWhen FILE is '-' or not provided, the veneer is read from standard input",
+		Long:  `Generate a declarative config blob from a single 'basic veneer' file.
+When FILE is '-' or not provided, the veneer is read from standard input.`,

OCPBUGS-1272: Add pipe support to "opm alpha render-veneer basic" #1026

OCPBUGS-1272: Add pipe support to "opm alpha render-veneer basic" #1026

Uh oh!

Conversation

oceanc80 commented Sep 27, 2022

Uh oh!

codecov bot commented Sep 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joelanford Sep 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ankitathomas left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

openshift-ci bot commented Sep 30, 2022

Uh oh!

openshift-ci-robot commented Oct 4, 2022

Uh oh!

Uh oh!

codecov bot commented Sep 27, 2022 •

edited

Loading

joelanford Sep 28, 2022 •

edited

Loading