Repository: apigee/registry
Pull request #817 (2022-11-05 03:29:33)

Add --parent option to bulk upload commands and use configured project when --parent is not provided
Addresses #740

With this change, a `--parent` argument is accepted by the bulk upload commands in addition to `--project-id`, which is deprecated and produces a warning. If both are omitted, we get the parent from the `registry.project` and `registry.location` configuration values.

This is all figured out by a helper function in `upload/bulk.go`. I could imagine that moving up to a higher level to be used by other commands but leave that to be done when we are ready to use it.

codecov[bot]: # [Codecov](https://codecov.io/gh/apigee/registry/pull/817?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apigee) Report
> Merging [#817](https://codecov.io/gh/apigee/registry/pull/817?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apigee) (593a2db) into [main](https://codecov.io/gh/apigee/registry/commit/5c7ca08b76a71ab686d5ab9c5e125a00c7db4b57?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apigee) (5c7ca08) will **increase** coverage by `0.53%`.
> The diff coverage is `50.00%`.

```diff
@@            Coverage Diff             @@
##             main     #817      +/-   ##
==========================================
+ Coverage   55.29%   55.82%   +0.53%     
==========================================
  Files          97       97              
  Lines        8695     8750      +55     
==========================================
+ Hits         4808     4885      +77     
+ Misses       3406     3373      -33     
- Partials      481      492      +11     
```


| [Impacted Files](https://codecov.io/gh/apigee/registry/pull/817?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apigee) | Coverage Δ | |
|---|---|---|
| [cmd/registry/cmd/upload/bulk/discovery.go](https://codecov.io/gh/apigee/registry/pull/817/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apigee#diff-Y21kL3JlZ2lzdHJ5L2NtZC91cGxvYWQvYnVsay9kaXNjb3ZlcnkuZ28=) | `3.47% <0.00%> (+0.09%)` | :arrow_up: |
| [cmd/registry/cmd/upload/bulk/bulk.go](https://codecov.io/gh/apigee/registry/pull/817/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apigee#diff-Y21kL3JlZ2lzdHJ5L2NtZC91cGxvYWQvYnVsay9idWxrLmdv) | `53.33% <36.36%> (-46.67%)` | :arrow_down: |
| [cmd/registry/cmd/upload/bulk/protos.go](https://codecov.io/gh/apigee/registry/pull/817/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apigee#diff-Y21kL3JlZ2lzdHJ5L2NtZC91cGxvYWQvYnVsay9wcm90b3MuZ28=) | `66.40% <88.88%> (+0.62%)` | :arrow_up: |
| [cmd/registry/cmd/upload/bulk/openapi.go](https://codecov.io/gh/apigee/registry/pull/817/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apigee#diff-Y21kL3JlZ2lzdHJ5L2NtZC91cGxvYWQvYnVsay9vcGVuYXBpLmdv) | `65.80% <100.00%> (+0.82%)` | :arrow_up: |
| [cmd/registry/cmd/compute/conformance.go](https://codecov.io/gh/apigee/registry/pull/817/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apigee#diff-Y21kL3JlZ2lzdHJ5L2NtZC9jb21wdXRlL2NvbmZvcm1hbmNlLmdv) | `53.57% <0.00%> (-1.43%)` | :arrow_down: |
| [server/registry/actions\_specs.go](https://codecov.io/gh/apigee/registry/pull/817/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apigee#diff-c2VydmVyL3JlZ2lzdHJ5L2FjdGlvbnNfc3BlY3MuZ28=) | `76.71% <0.00%> (+0.21%)` | :arrow_up: |
| [server/registry/server.go](https://codecov.io/gh/apigee/registry/pull/817/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apigee#diff-c2VydmVyL3JlZ2lzdHJ5L3NlcnZlci5nbw==) | `52.56% <0.00%> (+3.95%)` | :arrow_up: |
| ... and [8 more](https://codecov.io/gh/apigee/registry/pull/817/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apigee) | |

:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apigee)


Patch:
diff --git a/cmd/registry/cmd/upload/bulk/bulk.go b/cmd/registry/cmd/upload/bulk/bulk.go
index 6b2a4b7..e5115a8 100644
--- a/cmd/registry/cmd/upload/bulk/bulk.go
+++ b/cmd/registry/cmd/upload/bulk/bulk.go
@@ -15,6 +15,11 @@
 package bulk
 
 import (
+	"errors"
+	"fmt"
+
+	"github.com/apigee/registry/log"
+	"github.com/apigee/registry/pkg/connection"
 	"github.com/spf13/cobra"
 )
 
@@ -28,9 +33,39 @@ func Command() *cobra.Command {
 	cmd.AddCommand(openAPICommand())
 	cmd.AddCommand(protosCommand())
 
-	cmd.PersistentFlags().String("project-id", "", "Project ID to use for each upload")
-	_ = cmd.MarkFlagRequired("project-id")
-
+	cmd.PersistentFlags().String("project-id", "", "Project ID to use for each upload (deprecated)")
+	cmd.PersistentFlags().String("parent", "", "Parent for the upload (projects/PROJECT/locations/LOCATION)")
 	cmd.PersistentFlags().Int("jobs", 10, "Number of upload jobs to run simultaneously")
 	return cmd
 }
+
+func getParent(cmd *cobra.Command) (string, error) {
+	ctx := cmd.Context()
+
+	parent, err := cmd.Flags().GetString("parent")
+	if err != nil {
+		return "", fmt.Errorf("failed to get parent from flags (%s)", err)
+	}
+	projectID, err := cmd.Flags().GetString("project-id")
+	if err != nil {
+		return "", fmt.Errorf("failed to get project-id from flags (%s)", err)
+	}
+	if projectID != "" && parent != "" {
+		return "", errors.New("--project-id cannot be used with --parent")
+	}
+	if parent != "" {
+		return parent, nil
+	} else if projectID != "" {
+		log.FromContext(ctx).Warn("--project-id is deprecated, please use --parent or configure registry.project")
+		return "projects/" + projectID + "/locations/global", nil
+	}
+	c, err := connection.ActiveConfig()
+	if err != nil {
+		return "", fmt.Errorf("unable to identify parent (%s)", err)
+	}
+	parent, err = c.ProjectWithLocation()
+	if err != nil {
+		return "", fmt.Errorf("unable to identify parent: please use --parent or set registry.project in configuration (%s)", err)
+	}
+	return parent, nil
+}
diff --git a/cmd/registry/cmd/upload/bulk/discovery.go b/cmd/registry/cmd/upload/bulk/discovery.go
index eb2134f..1a8768d 100644
--- a/cmd/registry/cmd/upload/bulk/discovery.go
+++ b/cmd/registry/cmd/upload/bulk/discovery.go
@@ -37,11 +37,10 @@ func discoveryCommand() *cobra.Command {
 		Short: "Bulk-upload API Discovery documents from the Google API Discovery service",
 		Run: func(cmd *cobra.Command, args []string) {
 			ctx := cmd.Context()
-			projectID, err := cmd.Flags().GetString("project-id")
+			parent, err := getParent(cmd)
 			if err != nil {
-				log.FromContext(ctx).WithError(err).Fatal("Failed to get project-id from flags")
+				log.FromContext(ctx).WithError(err).Fatal("Failed to identify parent project")
 			}
-
 			client, err := connection.NewRegistryClient(ctx)
 			if err != nil {
 				log.FromContext(ctx).WithError(err).Fatal("Failed to get client")
@@ -65,7 +64,7 @@ func discoveryCommand() *cobra.Command {
 				taskQueue <- &uploadDiscoveryTask{
 					client:    client,
 					path:      api.DiscoveryRestURL,
-					projectID: projectID,
+					parent:    parent,
 					apiID:     sanitize(api.Name),
 					versionID: sanitize(api.Version),
 					specID:    "discovery.json",
@@ -80,7 +79,7 @@ func discoveryCommand() *cobra.Command {
 type uploadDiscoveryTask struct {
 	client    connection.RegistryClient
 	path      string
-	projectID string
+	parent    string
 	apiID     string
 	versionID string
 	specID    string
@@ -194,12 +193,8 @@ func (task *uploadDiscoveryTask) createOrUpdateSpec(ctx context.Context) error {
 	return nil
 }
 
-func (task *uploadDiscoveryTask) projectName() string {
-	return fmt.Sprintf("projects/%s", task.projectID)
-}
-
 func (task *uploadDiscoveryTask) apiName() string {
-	return fmt.Sprintf("%s/locations/global/apis/%s", task.projectName(), task.apiID)
+	return fmt.Sprintf("%s/apis/%s", task.parent, task.apiID)
 }
 
 func (task *uploadDiscoveryTask) versionName() string {
diff --git a/cmd/registry/cmd/upload/bulk/openapi.go b/cmd/registry/cmd/upload/bulk/openapi.go
index b34b377..aee8cdd 100644
--- a/cmd/registry/cmd/upload/bulk/openapi.go
+++ b/cmd/registry/cmd/upload/bulk/openapi.go
@@ -41,11 +41,10 @@ func openAPICommand() *cobra.Command {
 		Args:  cobra.MinimumNArgs(1),
 		Run: func(cmd *cobra.Command, args []string) {
 			ctx := cmd.Context()
-			projectID, err := cmd.Flags().GetString("project-id")
+			parent, err := getParent(cmd)
 			if err != nil {
-				log.FromContext(ctx).WithError(err).Fatal("Failed to get project-id from flags")
+				log.FromContext(ctx).WithError(err).Fatal("Failed to identify parent project")
 			}
-
 			client, err := connection.NewRegistryClient(ctx)
 			if err != nil {
 				log.FromContext(ctx).WithError(err).Fatal("Failed to get client")
@@ -64,7 +63,7 @@ func openAPICommand() *cobra.Command {
 				if err != nil {
 					log.FromContext(ctx).WithError(err).Fatal("Invalid path")
 				}
-				scanDirectoryForOpenAPI(ctx, client, projectID, baseURI, path, taskQueue)
+				scanDirectoryForOpenAPI(ctx, client, parent, baseURI, path, taskQueue)
 			}
 		},
 	}
@@ -73,7 +72,7 @@ func openAPICommand() *cobra.Command {
 	return cmd
 }
 
-func scanDirectoryForOpenAPI(ctx context.Context, client connection.RegistryClient, projectID, baseURI, directory string, taskQueue chan<- core.Task) {
+func scanDirectoryForOpenAPI(ctx context.Context, client connection.RegistryClient, parent, baseURI, directory string, taskQueue chan<- core.Task) {
 	// walk a directory hierarchy, uploading every API spec that matches a set of expected file names.
 	if err := filepath.Walk(directory, func(path string, info os.FileInfo, err error) error {
 		if err != nil {
@@ -82,7 +81,7 @@ func scanDirectoryForOpenAPI(ctx context.Context, client connection.RegistryClie
 
 		task := &uploadOpenAPITask{
 			client:    client,
-			projectID: projectID,
+			parent:    parent,
 			baseURI:   baseURI,
 			path:      path,
 			directory: directory,
@@ -124,7 +123,7 @@ type uploadOpenAPITask struct {
 	path      string
 	directory string
 	version   string
-	projectID string
+	parent    string
 	apiID     string // computed at runtime
 	versionID string // computed at runtime
 	specID    string // computed at runtime
@@ -264,12 +263,8 @@ func (task *uploadOpenAPITask) createOrUpdateSpec(ctx context.Context) error {
 	return nil
 }
 
-func (task *uploadOpenAPITask) projectName() string {
-	return fmt.Sprintf("projects/%s", task.projectID)
-}
-
 func (task *uploadOpenAPITask) apiName() string {
-	return fmt.Sprintf("%s/locations/global/apis/%s", task.projectName(), task.apiID)
+	return fmt.Sprintf("%s/apis/%s", task.parent, task.apiID)
 }
 
 func (task *uploadOpenAPITask) versionName() string {
diff --git a/cmd/registry/cmd/upload/bulk/protos.go b/cmd/registry/cmd/upload/bulk/protos.go
index 453e1cd..8708b32 100644
--- a/cmd/registry/cmd/upload/bulk/protos.go
+++ b/cmd/registry/cmd/upload/bulk/protos.go
@@ -60,11 +60,10 @@ func protosCommand() *cobra.Command {
 		Args:  cobra.ExactArgs(1),
 		Run: func(cmd *cobra.Command, args []string) {
 			ctx := cmd.Context()
-			projectID, err := cmd.Flags().GetString("project-id")
+			parent, err := getParent(cmd)
 			if err != nil {
-				log.FromContext(ctx).WithError(err).Fatal("Failed to get project-id from flags")
+				log.FromContext(ctx).WithError(err).Fatal("Failed to identify parent project")
 			}
-
 			client, err := connection.NewRegistryClient(ctx)
 			if err != nil {
 				log.FromContext(ctx).WithError(err).Fatal("Failed to get client")
@@ -92,7 +91,7 @@ func protosCommand() *cobra.Command {
 					log.FromContext(ctx).WithError(err).Fatal("Invalid path")
 				}
 
-				if err := scanDirectoryForProtos(client, projectID, baseURI, path, root, taskQueue); err != nil {
+				if err := scanDirectoryForProtos(client, parent, baseURI, path, root, taskQueue); err != nil {
 					log.FromContext(ctx).WithError(err).Debug("Failed to walk directory")
 				}
 			}
@@ -105,15 +104,15 @@ func protosCommand() *cobra.Command {
 	return cmd
 }
 
-func scanDirectoryForProtos(client connection.RegistryClient, projectID, baseURI, start, root string, taskQueue chan<- core.Task) error {
+func scanDirectoryForProtos(client connection.RegistryClient, parent, baseURI, start, root string, taskQueue chan<- core.Task) error {
 	return filepath.Walk(start, func(filepath string, info os.FileInfo, err error) error {
 		if err != nil {
 			return err
 		}
 
 		// Skip everything that's not a YAML file in a versioned directory.
-		parent := path.Dir(filepath)
-		if info.IsDir() || !strings.HasSuffix(filepath, ".yaml") || !versionDirectory.MatchString(path.Base(parent)) {
+		container := path.Dir(filepath)
+		if info.IsDir() || !strings.HasSuffix(filepath, ".yaml") || !versionDirectory.MatchString(path.Base(container)) {
 			return nil
 		}
 
@@ -135,11 +134,11 @@ func scanDirectoryForProtos(client connection.RegistryClient, projectID, baseURI
 		taskQueue <- &uploadProtoTask{
 			client:         client,
 			baseURI:        baseURI,
-			projectID:      projectID,
+			parent:         parent,
 			apiID:          strings.TrimSuffix(sc.Name, ".googleapis.com"),
 			apiTitle:       sc.Title,
 			apiDescription: strings.ReplaceAll(sc.Documentation.Summary, "\n", " "),
-			path:           parent,
+			path:           container,
 			directory:      root,
 		}
 
@@ -151,7 +150,7 @@ func scanDirectoryForProtos(client connection.RegistryClient, projectID, baseURI
 type uploadProtoTask struct {
 	client         connection.RegistryClient
 	baseURI        string
-	projectID      string
+	parent         string
 	path           string
 	directory      string
 	apiID          string
@@ -277,12 +276,8 @@ func (task *uploadProtoTask) createOrUpdateSpec(ctx context.Context) error {
 	return nil
 }
 
-func (task *uploadProtoTask) projectName() string {
-	return fmt.Sprintf("projects/%s", task.projectID)
-}
-
 func (task *uploadProtoTask) apiName() string {
-	return fmt.Sprintf("%s/locations/global/apis/%s", task.projectName(), task.apiID)
+	return fmt.Sprintf("%s/apis/%s", task.parent, task.apiID)
 }
 
 func (task *uploadProtoTask) versionName() string {


Test patch:
diff --git a/cmd/registry/cmd/upload/bulk/bulk_test.go b/cmd/registry/cmd/upload/bulk/bulk_test.go
new file mode 100644
index 0000000..71f0469
--- /dev/null
+++ b/cmd/registry/cmd/upload/bulk/bulk_test.go
@@ -0,0 +1,130 @@
+// Copyright 2022 Google LLC. All Rights Reserved.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//    http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+package bulk
+
+import (
+	"context"
+	"testing"
+
+	"github.com/apigee/registry/pkg/connection"
+	"github.com/apigee/registry/pkg/connection/grpctest"
+	"github.com/apigee/registry/server/registry"
+)
+
+// TestMain will set up a local RegistryServer and grpc.Server for all
+// tests in this package if APG_REGISTRY_ADDRESS env var is not set
+// for the client.
+func TestMain(m *testing.M) {
+	grpctest.TestMain(m, registry.Config{})
+}
+
+func TestParentFromFlags(t *testing.T) {
+	tests := []struct {
+		desc  string
+		args  []string
+		want  string
+		fails bool
+	}{
+		{
+			desc: "parent from deprecated projectid flag",
+			args: []string{"--project-id", "sample"},
+			want: "projects/sample/locations/global",
+		},
+		{
+			desc: "parent from parent flag",
+			args: []string{"--parent", "projects/sample/locations/other"},
+			want: "projects/sample/locations/other",
+		},
+		{
+			desc:  "parent from projectid and parent flags",
+			args:  []string{"--parent", "projects/sample/locations/global", "--project-id", "sample"},
+			fails: true,
+		},
+	}
+	for _, test := range tests {
+		t.Run(test.desc, func(t *testing.T) {
+			cmd := Command()
+			cmd.SetContext(context.Background())
+			if err := cmd.ParseFlags(test.args); err != nil {
+				t.Fatalf("Failed to parse flags")
+			}
+			parent, err := getParent(cmd)
+			if err != nil {
+				if !test.fails {
+					t.Errorf("Get parent unexpectedly failed with error: %s", err)
+				}
+			} else {
+				if test.fails {
+					t.Errorf("Get parent unexpectedly succeeded")
+				} else if parent != test.want {
+					t.Errorf("Get parent: wanted %s, got %s", test.want, parent)
+				}
+			}
+		})
+	}
+}
+
+func TestParentFromConfiguration(t *testing.T) {
+	tests := []struct {
+		desc       string
+		projectId  string
+		locationId string
+		want       string
+		fails      bool
+	}{
+		{
+			desc:       "configured with specified location",
+			projectId:  "sample",
+			locationId: "other",
+			want:       "projects/sample/locations/other",
+		},
+		{
+			desc:       "configured with default location",
+			projectId:  "sample",
+			locationId: "",
+			want:       "projects/sample/locations/global",
+		},
+		{
+			desc:       "configured with no location",
+			projectId:  "",
+			locationId: "",
+			want:       "",
+			fails:      true,
+		},
+	}
+	for _, test := range tests {
+		t.Run(test.desc, func(t *testing.T) {
+			config, err := connection.ActiveConfig()
+			if err != nil {
+				t.Fatalf("Setup: Failed to get registry configuration: %s", err)
+			}
+			config.Project = test.projectId
+			config.Location = test.locationId
+			connection.SetConfig(config)
+			cmd := Command()
+			cmd.SetContext(context.Background())
+			if err := cmd.ParseFlags([]string{}); err != nil {
+				t.Fatalf("Failed to parse flags")
+			}
+			parent, err := getParent(cmd)
+			if test.fails && err == nil {
+				t.Errorf("Get parent was expected to fail and didn't")
+			} else if !test.fails && err != nil {
+				t.Errorf("Get parent unexpectedly failed with error: %s", err)
+			} else if parent != test.want {
+				t.Errorf("Get parent: wanted %s, got %s", test.want, parent)
+			}
+		})
+	}
+}
diff --git a/cmd/registry/cmd/upload/bulk/openapi_test.go b/cmd/registry/cmd/upload/bulk/openapi_test.go
index fb623eb..724ece7 100644
--- a/cmd/registry/cmd/upload/bulk/openapi_test.go
+++ b/cmd/registry/cmd/upload/bulk/openapi_test.go
@@ -19,24 +19,16 @@ import (
 	"testing"
 
 	"github.com/apigee/registry/pkg/connection"
-	"github.com/apigee/registry/pkg/connection/grpctest"
 	"github.com/apigee/registry/rpc"
-	"github.com/apigee/registry/server/registry"
 	"google.golang.org/grpc/codes"
 	"google.golang.org/grpc/status"
 )
 
-// TestMain will set up a local RegistryServer and grpc.Server for all
-// tests in this package if APG_REGISTRY_ADDRESS env var is not set
-// for the client.
-func TestMain(m *testing.M) {
-	grpctest.TestMain(m, registry.Config{})
-}
-
 func TestOpenAPI(t *testing.T) {
 	const (
 		projectID   = "openapi-test"
 		projectName = "projects/" + projectID
+		parent      = projectName + "/locations/global"
 	)
 	// Create a registry client.
 	ctx := context.Background()
@@ -70,7 +62,7 @@ func TestOpenAPI(t *testing.T) {
 		t.Fatalf("Error creating project %s", err)
 	}
 	cmd := Command()
-	args := []string{"openapi", "testdata/openapi", "--project-id", projectID}
+	args := []string{"openapi", "testdata/openapi", "--parent", parent}
 	cmd.SetArgs(args)
 	if err := cmd.Execute(); err != nil {
 		t.Fatalf("Execute() with args %+v returned error: %s", args, err)
diff --git a/cmd/registry/cmd/upload/bulk/protos_test.go b/cmd/registry/cmd/upload/bulk/protos_test.go
index a8a31dc..2589286 100644
--- a/cmd/registry/cmd/upload/bulk/protos_test.go
+++ b/cmd/registry/cmd/upload/bulk/protos_test.go
@@ -32,6 +32,7 @@ func TestProtos(t *testing.T) {
 	const (
 		projectID   = "protos-test"
 		projectName = "projects/" + projectID
+		parent      = projectName + "/locations/global"
 	)
 	// Create a registry client.
 	ctx := context.Background()
@@ -65,7 +66,7 @@ func TestProtos(t *testing.T) {
 		t.Fatalf("Error creating project %s", err)
 	}
 	cmd := Command()
-	args := []string{"protos", "testdata/protos", "--project-id", projectID}
+	args := []string{"protos", "testdata/protos", "--parent", parent}
 	cmd.SetArgs(args)
 	if err := cmd.Execute(); err != nil {
 		t.Fatalf("Execute() with args %+v returned error: %s", args, err)