Replace input keys bundleDirKey and manifestsDirKey with private fields

bundleDirKey and manifestsDirKey were not suited to be generator input keys.
Much simpler to have them as private fields.
Plus some linter and cli docs cleanup.

Author: hasbro17

CHANGELOG.md

@@ -8,6 +8,7 @@
 - Add Prometheus metrics support to Ansible-based operators. ([#2179](https://github.com/operator-framework/operator-sdk/pull/2179))
 - On `generate csv`, populate a CSV manifest’s `spec.icon`, `spec.keywords`, and `spec.mantainers` fields with empty values to better inform users how to add data. ([#2521](https://github.com/operator-framework/operator-sdk/pull/2521))
 - Added the [`generate csv --include`](doc/cli/operator-sdk_generate_csv.md#options) option to include files as input to the CSV generator in lieu of a config. ([#2249](https://github.com/operator-framework/operator-sdk/pull/2249))
+- Added the [`generate csv --inputs`](doc/cli/operator-sdk_generate_csv.md#options) option to allow configuring input locations for operator manifests and API types directories and  input to the CSV generator in lieu of a config. ([#2511](https://github.com/operator-framework/operator-sdk/pull/2511))
 
 ### Changed
 - Ansible scaffolding has been rewritten to be simpler and make use of newer features of Ansible and Molecule.
@@ -23,7 +24,7 @@
 
 ### Removed
 
-- **Breaking Change:** Removed CSV configuration file support (defaulting to deploy/olm-catalog/csv-config.yaml) in favor of including files as input to the generator using [`generate csv --include`](doc/cli/operator-sdk_generate_csv.md#options), defaulting to the `deploy/` directory. ([#2249](https://github.com/operator-framework/operator-sdk/pull/2249))
+- **Breaking Change:** Removed CSV configuration file support (defaulting to deploy/olm-catalog/csv-config.yaml) in favor of specifying inputs to the generator via [`generate csv --input`](doc/cli/operator-sdk_generate_csv.md#options), and configuring output locations via [`generate csv --outputDir`](doc/cli/operator-sdk_generate_csv.md#options). ([#2511](https://github.com/operator-framework/operator-sdk/pull/2511))
 
 ### Bug Fixes
 

cmd/operator-sdk/generate/csv.go

@@ -89,8 +89,8 @@ version to --from-version. Otherwise the SDK will scaffold a new CSV manifest.`,
 Use this to set custom paths for operator manifests and API type definitions
 E.g: --inputs deploy=config/production,apis=pkg/myapp/apis 
 Supported input keys:
-	- deploy=<path to root directory for operator manifests (Deployment, RBAC, CRDs)>
-	- apis=<path to root directory for API type defintions>
+	- deploy=<project-relative path to root directory for operator manifests (Deployment, RBAC, CRDs)>
+	- apis=<project-relative path to root directory for API type defintions>
 `)
 	cmd.Flags().StringVar(&c.outputDir, "output-dir", scaffold.DeployDir,
 		"Base directory to output generated CSV. The resulting CSV bundle directory"+
@@ -139,7 +139,7 @@ func (c csvCmd) run() error {
 		if err != nil {
 			return fmt.Errorf("failed to update CRD's: %v", err)
 		}
-		// TODO: This path should come from the CSV generator
+		// TODO: This path should come from the CSV generator field csvOutputDir
 		bundleDir := filepath.Join(c.outputDir, gencatalog.OLMCatalogChildDir, c.operatorName, c.csvVersion)
 		for path, b := range crdManifestSet {
 			path = filepath.Join(bundleDir, path)

doc/cli/operator-sdk_generate_csv.md

@@ -18,15 +18,21 @@ operator-sdk generate csv [flags]
 ### Options
 
 ```
-      --csv-channel string     Channel the CSV should be registered under in the package manifest
-      --csv-version string     Semantic version of the CSV
-      --default-channel        Use the channel passed to --csv-channel as the package manifests' default channel. Only valid when --csv-channel is set
-      --from-version string    Semantic version of an existing CSV to use as a base
-  -h, --help                   help for csv
-      --include strings        Paths to include in CSV generation, ex. "deploy/prod,deploy/test". If this flag is set and you want to enable default behavior, you must include "deploy/" in the argument list (default [deploy])
-      --operator-name string   Operator name to use while generating CSV
-      --output-dir string      Base directory to output generated CSV. The resulting CSV bundle directory will be "<output-dir>/olm-catalog/<operator-name>/<csv-version>" (default "deploy")
-      --update-crds            Update CRD manifests in deploy/{operator-name}/{csv-version} the using latest API's
+      --csv-channel string      Channel the CSV should be registered under in the package manifest
+      --csv-version string      Semantic version of the CSV
+      --default-channel         Use the channel passed to --csv-channel as the package manifests' default channel. Only valid when --csv-channel is set
+      --from-version string     Semantic version of an existing CSV to use as a base
+  -h, --help                    help for csv
+      --inputs stringToString   Key value input paths used in CSV generation.
+                                Use this to set custom paths for operator manifests and API type definitions
+                                E.g: --inputs deploy=config/production,apis=pkg/myapp/apis 
+                                Supported input keys:
+                                	- deploy=<project-relative path to root directory for operator manifests (Deployment, RBAC, CRDs)>
+                                	- apis=<project-relative path to root directory for API type defintions>
+                                 (default [apis=pkg/apis,deploy=deploy])
+      --operator-name string    Operator name to use while generating CSV
+      --output-dir string       Base directory to output generated CSV. The resulting CSV bundle directorywill be "<output-dir>/olm-catalog/<operator-name>/<csv-version>" (default "deploy")
+      --update-crds             Update CRD manifests in deploy/{operator-name}/{csv-version} the using latest API's
 ```
 
 ### SEE ALSO

internal/generate/olm-catalog/csv.go

@@ -51,9 +51,6 @@ const (
 
 	// Input keys for CSV generator whose values are the filepaths for the respective input directories
 
-	// DEBUG: Why is this an input key? Users don't need to configure this.
-	// bundleDirKey is for the location of the bundle manifests directory
-	bundleDirKey = "bundle"
 	// DeployDirKey is for the location of the operator manifests directory e.g "deploy/production"
 	DeployDirKey = "deploy"
 	// APIsDirKey is for the location of the API types directory e.g "pkg/apis"
@@ -68,6 +65,12 @@ type csvGenerator struct {
 	// manifest with this version should exist at:
 	// deploy/olm-catalog/{from_version}/operator-name.v{from_version}.{csvYamlFileExt}
 	fromVersion string
+	// existingCSVBundleDir is set if the generator needs to update from
+	// an existing CSV bundle directory
+	existingCSVBundleDir string
+	// csvOutputDir is the bundle directory filepath where the CSV will be generated
+	// This is set according to the generator's OutputDir
+	csvOutputDir string
 }
 
 func NewCSV(cfg gen.Config, csvVersion, fromVersion string) gen.Generator {
@@ -88,22 +91,23 @@ func NewCSV(cfg gen.Config, csvVersion, fromVersion string) gen.Generator {
 		// Set to non-standard location.
 		olmCatalogDir = filepath.Join(g.Inputs[DeployDirKey], OLMCatalogChildDir)
 	}
-	if bundle, ok := g.Inputs[bundleDirKey]; !ok || bundle == "" {
-		// Initialize so we don't have to check for key existence elsewhere.
-		g.Inputs[bundleDirKey] = ""
-		parentDir := filepath.Join(olmCatalogDir, g.OperatorName)
-		if isBundleDirExist(parentDir, g.fromVersion) {
-			g.Inputs[bundleDirKey] = filepath.Join(olmCatalogDir, g.OperatorName, g.fromVersion)
-		} else if isBundleDirExist(parentDir, g.csvVersion) {
-			g.Inputs[bundleDirKey] = filepath.Join(olmCatalogDir, g.OperatorName, g.csvVersion)
-		}
+
+	parentDir := filepath.Join(olmCatalogDir, g.OperatorName)
+	if isBundleDirExist(parentDir, g.fromVersion) {
+		g.existingCSVBundleDir = filepath.Join(olmCatalogDir, g.OperatorName, g.fromVersion)
+	} else if isBundleDirExist(parentDir, g.csvVersion) {
+		g.existingCSVBundleDir = filepath.Join(olmCatalogDir, g.OperatorName, g.csvVersion)
 	}
+
 	if apisDir, ok := g.Inputs[APIsDirKey]; !ok || apisDir == "" {
 		g.Inputs[APIsDirKey] = scaffold.ApisDir
 	}
 	if g.OutputDir == "" {
 		g.OutputDir = scaffold.DeployDir
 	}
+
+	// Set the CSV bundle dir output path under the generator's OutputDir
+	g.csvOutputDir = filepath.Join(g.OutputDir, OLMCatalogChildDir, g.OperatorName, g.csvVersion)
 	return g
 }
 
@@ -112,8 +116,13 @@ func isBundleDirExist(parentDir, version string) bool {
 	if parentDir == "" || version == "" {
 		return false
 	}
-	_, err := os.Stat(filepath.Join(parentDir, version))
-	return err == nil || os.IsExist(err)
+	bundleDir := filepath.Join(parentDir, version)
+	_, err := os.Stat(bundleDir)
+	if !os.IsNotExist(err) {
+		// TODO: return and handle this error
+		log.Fatalf("Failed to stat existing bundle directory %s: %v", bundleDir, err)
+	}
+	return err == nil
 }
 
 func getCSVName(name, version string) string {
@@ -135,12 +144,11 @@ func (g csvGenerator) Generate() error {
 		return errors.New("error generating CSV manifest: no generated file found")
 	}
 
-	csvOutputDir := filepath.Join(g.OutputDir, OLMCatalogChildDir, g.OperatorName, g.csvVersion)
-	if err = os.MkdirAll(csvOutputDir, fileutil.DefaultDirFileMode); err != nil {
-		return errors.Wrapf(err, "error mkdir %s", csvOutputDir)
+	if err = os.MkdirAll(g.csvOutputDir, fileutil.DefaultDirFileMode); err != nil {
+		return errors.Wrapf(err, "error mkdir %s", g.csvOutputDir)
 	}
 	for fileName, b := range fileMap {
-		path := filepath.Join(csvOutputDir, fileName)
+		path := filepath.Join(g.csvOutputDir, fileName)
 		log.Debugf("CSV generator writing %s", path)
 		if err = ioutil.WriteFile(path, b, fileutil.DefaultFileMode); err != nil {
 			return err
@@ -150,11 +158,12 @@ func (g csvGenerator) Generate() error {
 }
 
 func (g csvGenerator) generate() (fileMap map[string][]byte, err error) {
-	bundle := g.Inputs[bundleDirKey]
 	// Get current CSV to update, otherwise start with a fresh CSV.
 	var csv *olmapiv1alpha1.ClusterServiceVersion
-	if bundle != "" {
-		if csv, err = getCSVFromDir(bundle); err != nil {
+	if g.existingCSVBundleDir != "" {
+		// TODO: If bundle dir exists, but the CSV file does not
+		// then we should create a new one and not return an error.
+		if csv, err = getCSVFromDir(g.existingCSVBundleDir); err != nil {
 			return nil, err
 		}
 		// TODO: validate existing CSV.
@@ -173,7 +182,8 @@ func (g csvGenerator) generate() (fileMap map[string][]byte, err error) {
 
 	path := getCSVFileName(g.OperatorName, g.csvVersion)
 	if fields := getEmptyRequiredCSVFields(csv); len(fields) != 0 {
-		if bundle != "" {
+		if g.existingCSVBundleDir != "" {
+			// An existing csv should have several required fields populated.
 			log.Warnf("Required csv fields not filled in file %s:%s\n", path, joinFields(fields))
 		} else {
 			// A new csv won't have several required fields populated.
@@ -357,8 +367,6 @@ func (g csvGenerator) updateCSVFromManifests(csv *olmapiv1alpha1.ClusterServiceV
 	kindManifestMap := map[schema.GroupVersionKind][][]byte{}
 	crGVKSet := map[schema.GroupVersionKind]struct{}{}
 	err = filepath.Walk(g.Inputs[DeployDirKey], func(path string, info os.FileInfo, werr error) error {
-		// DEBUG: How is this walking to read deploy/crds/... manifests
-		// if it's not walking directories?
 		if werr != nil || info.IsDir() {
 			return werr
 		}

internal/generate/olm-catalog/csv_go_test.go

@@ -15,7 +15,6 @@
 package olmcatalog
 
 import (
-	"fmt"
 	"io/ioutil"
 	"os"
 	"path/filepath"
@@ -35,16 +34,10 @@ import (
 )
 
 const (
-	sdkCmd = "operator-sdk"
-
-	testProjectName  = "memcached-operator"
-	csvVersion       = "0.0.3"
-	fromVersion      = "0.0.2"
-	notExistVersion  = "1.0.0"
-	scratchBundleDir = "scratch"
-	testGroup        = "cache.example.com"
-	testKind1        = "Memcached"
-	testVersion1     = "v1alpha1"
+	testProjectName = "memcached-operator"
+	csvVersion      = "0.0.3"
+	fromVersion     = "0.0.2"
+	notExistVersion = "1.0.0"
 )
 
 var (
@@ -68,7 +61,7 @@ func setupTestEnvWithCleanup(t *testing.T, dataDir string) (cleanupFuncs []func(
 	return cleanupFuncs
 }
 
-func TestGoCSVGoNew(t *testing.T) {
+func TestGoCSVFromNew(t *testing.T) {
 	for _, cleanupFunc := range setupTestEnvWithCleanup(t, testGoDataDir) {
 		defer cleanupFunc()
 	}
@@ -77,6 +70,8 @@ func TestGoCSVGoNew(t *testing.T) {
 		OperatorName: testProjectName,
 	}
 	g := NewCSV(cfg, csvVersion, "")
+	// TODO: Expand this test to test the g.Generate() method
+	// so we can test the CSV generator's input/out paths.
 	fileMap, err := g.(csvGenerator).generate()
 	if err != nil {
 		t.Fatalf("Failed to execute CSV generator: %v", err)
@@ -153,14 +148,6 @@ func TestGoCSVIncludeAll(t *testing.T) {
 	}
 }
 
-func getTestCRDFile(g, k string) string {
-	return fmt.Sprintf("%s_%s_crd.yaml", g, strings.ToLower(k)+"s")
-}
-
-func getTestCRFile(g, v, k string) string {
-	return fmt.Sprintf("%s_%s_%s_cr.yaml", g, v, strings.ToLower(k))
-}
-
 func TestUpdateVersion(t *testing.T) {
 	csv, err := getCSVFromDir(filepath.Join(testGoDataDir, OLMCatalogDir, testProjectName, fromVersion))
 	if err != nil {

internal/generate/olm-catalog/csv_updaters.go

@@ -269,7 +269,9 @@ func (us crds) apply(csv *olmapiv1alpha1.ClusterServiceVersion) error {
 	return nil
 }
 
-func updateDescriptions(csv *olmapiv1alpha1.ClusterServiceVersion, searchDir string, opType projutil.OperatorType) error {
+func updateDescriptions(csv *olmapiv1alpha1.ClusterServiceVersion,
+	searchDir string,
+	opType projutil.OperatorType) error {
 	ownedCRDs := []olmapiv1alpha1.CRDDescription{}
 	for _, ownedCRD := range csv.Spec.CustomResourceDefinitions.Owned {
 		name := ownedCRD.Name

internal/generate/olm-catalog/package_manifest.go

@@ -35,9 +35,6 @@ import (
 
 const (
 	packageManifestFileExt = ".package.yaml"
-
-	// TODO: Is this an input that needs to be set via the csv generator inputs flag?
-	manifestsDirKey = "manifests"
 )
 
 type pkgGenerator struct {
@@ -52,6 +49,10 @@ type pkgGenerator struct {
 	channelIsDefault bool
 	// PackageManifest file name
 	fileName string
+
+	// existingPkgManifestDir is set to the package manifest root directory
+	// if the generator needs to update from an existing package manifest file.
+	existingPkgManifestDir string
 }
 
 func NewPackageManifest(cfg gen.Config, csvVersion, channel string, isDefault bool) gen.Generator {
@@ -73,8 +74,11 @@ func NewPackageManifest(cfg gen.Config, csvVersion, channel string, isDefault bo
 		olmCatalogDir = filepath.Join(g.Inputs[DeployDirKey], OLMCatalogChildDir)
 	}
 
-	if manifests, ok := g.Inputs[manifestsDirKey]; !ok || manifests == "" {
-		g.Inputs[manifestsDirKey] = filepath.Join(olmCatalogDir, g.OperatorName)
+	// Check if the generator should update from an existing package manifest file
+	pkgManifestDirPath := filepath.Join(olmCatalogDir, g.OperatorName)
+	pkgManifestFilePath := filepath.Join(pkgManifestDirPath, g.fileName)
+	if isFileExist(pkgManifestFilePath) {
+		g.existingPkgManifestDir = pkgManifestDirPath
 	}
 
 	if g.OutputDir == "" {
@@ -83,6 +87,15 @@ func NewPackageManifest(cfg gen.Config, csvVersion, channel string, isDefault bo
 	return g
 }
 
+func isFileExist(path string) bool {
+	_, err := os.Stat(path)
+	if !os.IsNotExist(err) {
+		// TODO: return and handle this error
+		log.Fatalf("Failed to stat %s: %v", path, err)
+	}
+	return err == nil
+}
+
 // getPkgFileName will return the name of the PackageManifestFile
 func getPkgFileName(operatorName string) string {
 	return strings.ToLower(operatorName) + packageManifestFileExt
@@ -138,23 +151,21 @@ func (g pkgGenerator) generate() (map[string][]byte, error) {
 	return fileMap, nil
 }
 
-// buildPackageManifest will create a registry.PackageManifest from scratch, or modify
+// buildPackageManifest will create a registry.PackageManifest from scratch, or reads
 // an existing one if found at the expected path.
 func (g pkgGenerator) buildPackageManifest() (registry.PackageManifest, error) {
 	pkg := registry.PackageManifest{}
-	path := filepath.Join(g.Inputs[manifestsDirKey], g.fileName)
-	if _, err := os.Stat(path); err == nil {
-		b, err := ioutil.ReadFile(path)
+	if g.existingPkgManifestDir != "" {
+		existingPkgManifest := filepath.Join(g.existingPkgManifestDir, g.fileName)
+		b, err := ioutil.ReadFile(existingPkgManifest)
 		if err != nil {
-			return pkg, fmt.Errorf("failed to read package manifest %s: %v", path, err)
+			return pkg, fmt.Errorf("failed to read package manifest %s: %v", existingPkgManifest, err)
 		}
 		if err = yaml.Unmarshal(b, &pkg); err != nil {
-			return pkg, fmt.Errorf("failed to unmarshal package manifest %s: %v", path, err)
+			return pkg, fmt.Errorf("failed to unmarshal package manifest %s: %v", existingPkgManifest, err)
 		}
-	} else if os.IsNotExist(err) {
-		pkg = newPackageManifest(g.OperatorName, g.channel, g.csvVersion)
 	} else {
-		return pkg, fmt.Errorf("error reading package manifest %s: %v", path, err)
+		pkg = newPackageManifest(g.OperatorName, g.channel, g.csvVersion)
 	}
 	return pkg, nil
 }

internal/generate/olm-catalog/package_manifest_test.go

@@ -27,13 +27,19 @@ import (
 
 // newTestPackageManifestGenerator returns a package manifest Generator populated with test values.
 func newTestPackageManifestGenerator() gen.Generator {
-	inputDir := filepath.Join(testGoDataDir, OLMCatalogDir, testProjectName)
 	cfg := gen.Config{
 		OperatorName: testProjectName,
-		Inputs:       map[string]string{manifestsDirKey: inputDir},
 	}
 	g := NewPackageManifest(cfg, csvVersion, "stable", true)
-	return g
+
+	// TODO: The deploy dir input should be the way to point the generator to test data
+	// E.g: Inputs: map[string]string{DeployDirKey: testGoDataDir}
+
+	// Override the generator to point to the package manifest in testGoDataDir
+	testPkgManifestDir := filepath.Join(testGoDataDir, OLMCatalogDir, testProjectName)
+	pkgGen := g.(pkgGenerator)
+	pkgGen.existingPkgManifestDir = testPkgManifestDir
+	return pkgGen
 }
 
 func TestGeneratePackageManifest(t *testing.T) {