Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions alpha/declcfg/load.go
Original file line number Diff line number Diff line change
Expand Up @@ -118,7 +118,9 @@ func extractCSV(objs []string) string {
return ""
}

func readYAMLOrJSON(r io.Reader) (*DeclarativeConfig, error) {
// LoadReader reads yaml or json from the passed in io.Reader and unmarshals it into a DeclarativeConfig struct.
// Path references will not be de-referenced so callers are responsible for de-referencing if necessary.
func LoadReader(r io.Reader) (*DeclarativeConfig, error) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

cfg := &DeclarativeConfig{}
dec := yaml.NewYAMLOrJSONDecoder(r, 4096)
for {
Expand Down Expand Up @@ -173,7 +175,7 @@ func LoadFile(root fs.FS, path string) (*DeclarativeConfig, error) {
}
defer file.Close()

cfg, err := readYAMLOrJSON(file)
cfg, err := LoadReader(file)
if err != nil {
return nil, err
}
Expand Down
4 changes: 2 additions & 2 deletions alpha/declcfg/load_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ import (
"github.com/operator-framework/operator-registry/alpha/property"
)

func TestReadYAMLOrJSON(t *testing.T) {
func TestLoadReader(t *testing.T) {
type spec struct {
name string
fsys fs.FS
Expand Down Expand Up @@ -80,7 +80,7 @@ func TestReadYAMLOrJSON(t *testing.T) {
f, err := s.fsys.Open(s.path)
require.NoError(t, err)

cfg, err := readYAMLOrJSON(f)
cfg, err := LoadReader(f)
s.assertion(t, err)
if err == nil {
require.NotNil(t, cfg)
Expand Down
39 changes: 8 additions & 31 deletions alpha/veneer/basic/basic.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,8 @@ package basic

import (
"context"
"errors"
"os"
"path/filepath"
"fmt"
"io"

"github.com/operator-framework/operator-registry/alpha/action"
"github.com/operator-framework/operator-registry/alpha/declcfg"
Expand All @@ -15,30 +14,8 @@ type Veneer struct {
Registry image.Registry
}

func (v Veneer) Render(ctx context.Context, ref string) (*declcfg.DeclarativeConfig, error) {
// only taking first argument as file
stat, serr := os.Stat(ref)
if serr != nil {
return nil, serr
}

if stat.IsDir() {
return nil, errors.New("cannot render veneers by directory reference")
}
return v.renderFile(ctx, ref)
}

func (v Veneer) renderFile(ctx context.Context, ref string) (*declcfg.DeclarativeConfig, error) {
// xform any relative to absolute paths
abspath, err := filepath.Abs(ref)
if err != nil {
return nil, err
}
// xform to break apart dir/file elements
rpath, fname := filepath.Split(abspath)
root := os.DirFS(rpath)

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@joelanford joelanford Sep 28, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

if err != nil {
return cfg, err
}
Expand All @@ -52,8 +29,7 @@ func (v Veneer) renderFile(ctx context.Context, ref string) (*declcfg.Declarativ

for _, b := range cfg.Bundles {
if !isBundleVeneer(&b) {
outb = append(outb, b)
continue
return nil, fmt.Errorf("unexpected fields present in basic veneer bundle")
}
r.Refs = []string{b.Image}
contributor, err := r.Run(ctx)
Expand All @@ -67,7 +43,8 @@ func (v Veneer) renderFile(ctx context.Context, ref string) (*declcfg.Declarativ
return cfg, nil
}

// isBundleVeneer identifies loaded partial Bundle data from YAML/JSON veneer source as having no properties,
// isBundleVeneer identifies a Bundle veneer source as having a Schema and Image defined
// 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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}
21 changes: 16 additions & 5 deletions cmd/opm/alpha/veneer/basic.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,11 +20,22 @@ func newBasicVeneerRenderCmd() *cobra.Command {
output string
)
cmd := &cobra.Command{
Use: "basic basic-veneer-file",
Short: "Generate a declarative config blob from a single 'basic veneer' file",
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),
Use: "basic basic-veneer-file",
Short: `Generate a file-based catalog from a single 'basic veneer' file
When FILE is '-' or not provided, the veneer is read from standard input`,
Long: `Generate a file-based catalog from a single 'basic veneer' file
When FILE is '-' or not provided, the veneer is read from standard input`,
Args: cobra.MaximumNArgs(1),
Run: func(cmd *cobra.Command, args []string) {
// Handle different input argument types
// When no arguments or "-" is passed to the command,
// assume input is coming from stdin
// Otherwise open the file passed to the command
data, source, err := openFileOrStdin(cmd, args)
if err != nil {
log.Fatalf("unable to open %q: %v", source, err)
}
defer data.Close()

var write func(declcfg.DeclarativeConfig, io.Writer) error
switch output {
Expand All @@ -50,7 +61,7 @@ func newBasicVeneerRenderCmd() *cobra.Command {
veneer.Registry = reg

// only taking first file argument
cfg, err := veneer.Render(cmd.Context(), args[0])
cfg, err := veneer.Render(cmd.Context(), data)
if err != nil {
log.Fatal(err)
}
Expand Down
11 changes: 11 additions & 0 deletions cmd/opm/alpha/veneer/cmd.go
Original file line number Diff line number Diff line change
@@ -1,6 +1,9 @@
package veneer

import (
"io"
"os"

"github.com/spf13/cobra"
)

Expand All @@ -16,3 +19,11 @@ func NewCmd() *cobra.Command {

return runCmd
}

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
}
Comment on lines +23 to +29
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yay, re-use!

28 changes: 8 additions & 20 deletions cmd/opm/alpha/veneer/semver.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,19 +18,22 @@ import (
func newSemverCmd() *cobra.Command {
output := ""
cmd := &cobra.Command{
Use: "semver [FILE]",
Short: "Generate a file-based catalog from a single 'semver veneer' file \nWhen FILE is '-' or not provided, the veneer is read from standard input",
Long: "Generate a file-based catalog from a single 'semver veneer' file \nWhen FILE is '-' or not provided, the veneer is read from standard input",
Args: cobra.MaximumNArgs(1),
Use: "semver [FILE]",
Short: `Generate a file-based catalog from a single 'semver veneer' file
When FILE is '-' or not provided, the veneer is read from standard input`,
Long: `Generate a file-based catalog from a single 'semver veneer' file
When FILE is '-' or not provided, the veneer is read from standard input`,
Args: cobra.MaximumNArgs(1),
RunE: func(cmd *cobra.Command, args []string) error {
// Handle different input argument types
// When no arguments or "-" is passed to the command,
// assume input is coming from stdin
// Otherwise open the file passed to the command
data, source, err := openFileOrReadStdin(cmd, args)
data, source, err := openFileOrStdin(cmd, args)
if err != nil {
return err
}
defer data.Close()

var write func(declcfg.DeclarativeConfig, io.Writer) error
switch output {
Expand Down Expand Up @@ -80,18 +83,3 @@ func newSemverCmd() *cobra.Command {
cmd.Flags().StringVarP(&output, "output", "o", "json", "Output format (json|yaml|mermaid)")
return cmd
}

func openFileOrReadStdin(cmd *cobra.Command, args []string) (io.Reader, string, error) {
var err error = nil
var source string = ""
var reader io.Reader

if len(args) == 0 || args[0] == "-" {
reader = cmd.InOrStdin()
source = "stdin"
} else {
reader, err = os.Open(args[0])
source = args[0]
}
return reader, source, err
}