fix(internal/sidekick/surfer): always specify primary (#5898)

Always specify is_primary_resource

Fix #5875

Author: sarahheacock

doc/gcloud/gcloud-declarative-schema.md

@@ -17,23 +17,19 @@ This document describes the schema for the gcloud Declarative YAML.
 | `help_text` | string | Is the help text shown for the argument in gcloud help and error messages. |
 | `action` | string | Overrides the argparse action used for the argument, for example store_true or store_true_false. Typically used with boolean fields. |
 | `is_positional` | bool | Makes the argument positional rather than a flag. |
-| `is_primary_resource` | bool | Marks this argument as the primary resource the command operates on. At most one argument per command may set this. |
-| `request_id_field` | string | Is the request-message field that receives the user-supplied resource ID on a create command, for example instanceId. |
-| `resource_spec` | [ResourceSpec](#resourcespec-configuration) (optional) | Declares this argument as a gcloud concept resource and describes its collection and attributes. |
 | `required` | bool | Makes the argument mandatory. gcloud rejects invocations that leave it unset. |
 | `repeated` | bool | Accepts more than one value for the argument and maps to a repeated API field. |
 | `clearable` | bool | Causes gcloud to also generate companion flags such as<br>--clear-..., --add-..., and --remove-... on update commands. |
 | `type` | string | Selects the gcloud argument parser, for example long, double, or a fully-qualified reference to an ArgDict or ArgList parser. |
 | `default` | [Default](#default-configuration) | Is the value used when the user does not supply the argument. |
 | `choices` | list of [Choice](#choice-configuration) | Is the fixed set of values the user can supply. Used for enum-typed fields; each choice maps an arg_value to an enum_value. |
 | `spec` | list of [ArgSpec](#argspec-configuration) | Lists the sub-fields of a structured argument such as a map. For a map field, Spec typically contains entries for key and value. |
-| `resource_method_params` | map[string]string | Maps API method parameter names to attribute names on a resource argument. Use it when the API method uses a non-standard parameter name for the resource. |
 
 ## Arguments Configuration
 
 | Field | Type | Description |
 | :--- | :--- | :--- |
-| `params` | list of [Argument](#argument-configuration) | Is the ordered list of arguments the command accepts. Each entry becomes either a positional argument or a flag. |
+| `params` | list of any | Is the ordered list of arguments the command accepts. Each entry becomes either a positional argument or a flag. |
 
 ## Async Configuration
 
@@ -103,6 +99,22 @@ This document describes the schema for the gcloud Declarative YAML.
 | `method` | string | Is the API method name. When empty, gcloud infers the method from the command type (for example, list for a list command). |
 | `static_fields` | map[string]string | Sets request fields to fixed values regardless of user input. Keys are dot-separated field paths in the request message. |
 
+## ResourceArg Configuration
+
+| Field | Type | Description |
+| :--- | :--- | :--- |
+| `arg_name` | string | Is the name of the argument as it appears to the user, such as instance. For flags gcloud prepends --. |
+| `help_text` | string | Is the help text shown for the argument in gcloud help and error messages. |
+| `is_positional` | bool | Makes the argument positional rather than a flag. |
+| `is_primary_resource` | bool | Marks this argument as the primary resource the command operates on. At most one argument per command may set this. |
+| `request_id_field` | string | Is the request-message field that receives the user-supplied resource ID on a create command, for example instanceId. |
+| `resource_spec` | [ResourceSpec](#resourcespec-configuration) (optional) | Declares this argument as a gcloud concept resource and describes its collection and attributes. |
+| `required` | bool | Makes the argument mandatory. gcloud rejects invocations that leave it unset. |
+| `repeated` | bool | Accepts more than one value for the argument and maps to a repeated API field. |
+| `clearable` | bool | Causes gcloud to also generate companion flags such as<br>--clear-..., --add-..., and --remove-... on update commands. |
+| `spec` | list of [ArgSpec](#argspec-configuration) | Lists the sub-fields of a structured argument such as a map. For a map field, Spec typically contains entries for key and value. |
+| `resource_method_params` | map[string]string | Maps API method parameter names to attribute names on a resource argument. Use it when the API method uses a non-standard parameter name for the resource. |
+
 ## ResourceSpec Configuration
 
 | Field | Type | Description |

internal/sidekick/surfer/command_writer.go

@@ -162,57 +162,77 @@ func mapCommandToYAML(c *Command, track ReleaseTrack) *declarative.Command {
 	return y
 }
 
-func mapArgumentsToYAML(args []Argument) []declarative.Argument {
-	var ya []declarative.Argument
+func mapArgumentsToYAML(args []Argument) []any {
+	var ya []any
 	for _, a := range args {
-		t := a.Type
-		if t == "str" {
-			t = ""
+		if a.ResourceSpec != nil {
+			ya = append(ya, mapResourceArgToYAML(a))
+		} else {
+			ya = append(ya, mapArgumentToYAML(a))
 		}
-		var def declarative.Default
-		if a.Action == "store_true" {
-			var nilVal any = nil
-			def.Value = &nilVal
-		}
-		var apiField string
-		var rmp map[string]string
-
-		if len(a.APIField) > 0 {
-			parts := make([]string, len(a.APIField))
-			for i, p := range a.APIField {
-				parts[i] = strcase.ToLowerCamel(p)
-			}
-			joinedPath := strings.Join(parts, ".")
-
-			if a.ResourceSpec != nil {
-				rmp = map[string]string{
-					joinedPath: "{__relative_name__}",
-				}
-			} else {
-				apiField = joinedPath
-			}
+	}
+	return ya
+}
+
+func mapResourceArgToYAML(a Argument) declarative.ResourceArg {
+	var rmp map[string]string
+	if path := apiFieldPath(a.APIField); path != "" {
+		rmp = map[string]string{
+			path: "{__relative_name__}",
 		}
+	}
 
-		ya = append(ya, declarative.Argument{
-			ArgName:              strcase.ToKebab(a.ArgName),
-			APIField:             apiField,
-			HelpText:             a.HelpText,
-			Action:               a.Action,
-			Default:              def,
-			IsPositional:         a.IsPositional,
-			IsPrimaryResource:    a.IsPrimaryResource,
-			RequestIDField:       a.RequestIDField,
-			ResourceSpec:         mapResourceSpecToYAML(a.ResourceSpec),
-			Required:             a.Required,
-			Repeated:             a.Repeated,
-			Clearable:            a.Clearable,
-			Type:                 t,
-			Choices:              mapChoicesToYAML(a.Choices),
-			Spec:                 mapArgSpecsToYAML(a.Spec),
-			ResourceMethodParams: rmp,
-		})
+	return declarative.ResourceArg{
+		ArgName:              strcase.ToKebab(a.ArgName),
+		HelpText:             a.HelpText,
+		IsPositional:         a.IsPositional,
+		IsPrimaryResource:    a.IsPrimaryResource,
+		RequestIDField:       a.RequestIDField,
+		ResourceSpec:         mapResourceSpecToYAML(a.ResourceSpec),
+		Required:             a.Required,
+		Repeated:             a.Repeated,
+		Clearable:            a.Clearable,
+		Spec:                 mapArgSpecsToYAML(a.Spec),
+		ResourceMethodParams: rmp,
 	}
-	return ya
+}
+
+func mapArgumentToYAML(a Argument) declarative.Argument {
+	t := a.Type
+	if t == "str" {
+		t = ""
+	}
+	var def declarative.Default
+	if a.Action == "store_true" {
+		var nilVal any = nil
+		def.Value = &nilVal
+	}
+
+	return declarative.Argument{
+		ArgName:      strcase.ToKebab(a.ArgName),
+		APIField:     apiFieldPath(a.APIField),
+		HelpText:     a.HelpText,
+		Action:       a.Action,
+		Default:      def,
+		IsPositional: a.IsPositional,
+		Required:     a.Required,
+		Repeated:     a.Repeated,
+		Clearable:    a.Clearable,
+		Type:         t,
+		Choices:      mapChoicesToYAML(a.Choices),
+		Spec:         mapArgSpecsToYAML(a.Spec),
+	}
+}
+
+func apiFieldPath(apiField []string) string {
+	if len(apiField) == 0 {
+		return ""
+	}
+	parts := make([]string, len(apiField))
+	for i, p := range apiField {
+		parts[i] = strcase.ToLowerCamel(p)
+	}
+	return strings.Join(parts, ".")
 }
 
 func mapResourceSpecToYAML(spec *ResourceSpec) *declarative.ResourceSpec {

internal/sidekick/surfer/command_writer_test.go

@@ -240,15 +240,13 @@ func TestMapCommandToYAML(t *testing.T) {
 			Format: "table",
 		},
 		Arguments: declarative.Arguments{
-			Params: []declarative.Argument{
-				{
+			Params: []any{
+				declarative.ResourceArg{
 					ArgName: "arg",
 					Spec: []declarative.ArgSpec{
 						{APIField: "s"},
 					},
-					Choices: []declarative.Choice{
-						{ArgValue: "av", EnumValue: "ev", HelpText: "ch"},
-					},
+					IsPrimaryResource: false,
 					ResourceSpec: &declarative.ResourceSpec{
 						Name:       "resourceName",
 						PluralName: "resourcePlural",
@@ -259,18 +257,19 @@ func TestMapCommandToYAML(t *testing.T) {
 						DisableAutoCompleters: true,
 					},
 				},
-				{
+				declarative.Argument{
 					ArgName: "bool-arg",
 					Type:    "bool",
 					Action:  "store_true",
 					Default: declarative.Default{Value: &nilVal},
 				},
-				{
+				declarative.Argument{
 					ArgName:  "arg-with-apifield",
 					APIField: "part1.part2",
 				},
-				{
-					ArgName: "arg-with-apifield-and-resource",
+				declarative.ResourceArg{
+					ArgName:           "arg-with-apifield-and-resource",
+					IsPrimaryResource: false,
 					ResourceSpec: &declarative.ResourceSpec{
 						Name: "nestedResource",
 					},

internal/sidekick/surfer/declarative/declarative.go

@@ -188,10 +188,10 @@ func (d Default) MarshalYAML() (any, error) {
 type Arguments struct {
 	// Params is the ordered list of arguments the command accepts. Each entry
 	// becomes either a positional argument or a flag.
-	Params []Argument `yaml:"params,omitempty"`
+	Params []any `yaml:"params,omitempty"`
 }
 
-// Argument describes a single command argument.
+// Argument describes a standard command argument (scalar flag or positional).
 type Argument struct {
 	// ArgName is the name of the argument as it appears to the user, such as
 	// instance-id. For flags gcloud prepends --.
@@ -212,18 +212,6 @@ type Argument struct {
 	// IsPositional makes the argument positional rather than a flag.
 	IsPositional bool `yaml:"is_positional"`
 
-	// IsPrimaryResource marks this argument as the primary resource the
-	// command operates on. At most one argument per command may set this.
-	IsPrimaryResource bool `yaml:"is_primary_resource,omitempty"`
-
-	// RequestIDField is the request-message field that receives the
-	// user-supplied resource ID on a create command, for example instanceId.
-	RequestIDField string `yaml:"request_id_field,omitempty"`
-
-	// ResourceSpec declares this argument as a gcloud concept resource and
-	// describes its collection and attributes.
-	ResourceSpec *ResourceSpec `yaml:"resource_spec,omitempty"`
-
 	// Required makes the argument mandatory. gcloud rejects invocations that
 	// leave it unset.
 	Required bool `yaml:"required"`
@@ -250,6 +238,48 @@ type Argument struct {
 	// Spec lists the sub-fields of a structured argument such as a map. For a
 	// map field, Spec typically contains entries for key and value.
 	Spec []ArgSpec `yaml:"spec,omitempty"`
+}
+
+// ResourceArg describes a concept-resource command argument.
+type ResourceArg struct {
+	// ArgName is the name of the argument as it appears to the user, such as
+	// instance. For flags gcloud prepends --.
+	ArgName string `yaml:"arg_name,omitempty"`
+
+	// HelpText is the help text shown for the argument in gcloud help and
+	// error messages.
+	HelpText string `yaml:"help_text"`
+
+	// IsPositional makes the argument positional rather than a flag.
+	IsPositional bool `yaml:"is_positional"`
+
+	// IsPrimaryResource marks this argument as the primary resource the
+	// command operates on. At most one argument per command may set this.
+	IsPrimaryResource bool `yaml:"is_primary_resource"`
+
+	// RequestIDField is the request-message field that receives the
+	// user-supplied resource ID on a create command, for example instanceId.
+	RequestIDField string `yaml:"request_id_field,omitempty"`
+
+	// ResourceSpec declares this argument as a gcloud concept resource and
+	// describes its collection and attributes.
+	ResourceSpec *ResourceSpec `yaml:"resource_spec,omitempty"`
+
+	// Required makes the argument mandatory. gcloud rejects invocations that
+	// leave it unset.
+	Required bool `yaml:"required"`
+
+	// Repeated accepts more than one value for the argument and maps to a
+	// repeated API field.
+	Repeated bool `yaml:"repeated,omitempty"`
+
+	// Clearable causes gcloud to also generate companion flags such as
+	// --clear-..., --add-..., and --remove-... on update commands.
+	Clearable bool `yaml:"clearable,omitempty"`
+
+	// Spec lists the sub-fields of a structured argument such as a map. For a
+	// map field, Spec typically contains entries for key and value.
+	Spec []ArgSpec `yaml:"spec,omitempty"`
 
 	// ResourceMethodParams maps API method parameter names to attribute names
 	// on a resource argument. Use it when the API method uses a non-standard

internal/surfer/testdata/apis/developerconnect/expected/current/surface/developerconnect/account_connectors/users/_partials/_fetch_access_token_ga.yaml

@@ -31,6 +31,7 @@
           The resource name of the AccountConnector in the format
           `projects/*/locations/*/accountConnectors/*`.
         is_positional: false
+        is_primary_resource: false
         resource_spec:
           attributes:
             - attribute_name: project

internal/surfer/testdata/apis/developerconnect/expected/current/surface/developerconnect/account_connectors/users/_partials/_fetch_self_ga.yaml

@@ -29,6 +29,7 @@
       - arg_name: name
         help_text: Name of the AccountConnector resource
         is_positional: false
+        is_primary_resource: false
         resource_spec:
           attributes:
             - attribute_name: project

internal/surfer/testdata/apis/developerconnect/expected/current/surface/developerconnect/account_connectors/users/_partials/_finish_o_auth_flow_ga.yaml

@@ -43,6 +43,7 @@
           The resource name of the AccountConnector in the format
           `projects/*/locations/*/accountConnectors/*`.
         is_positional: false
+        is_primary_resource: false
         resource_spec:
           attributes:
             - attribute_name: project

internal/surfer/testdata/apis/developerconnect/expected/current/surface/developerconnect/account_connectors/users/_partials/_start_o_auth_flow_ga.yaml

@@ -31,6 +31,7 @@
           The resource name of the AccountConnector in the format
           `projects/*/locations/*/accountConnectors/*`.
         is_positional: false
+        is_primary_resource: false
         resource_spec:
           attributes:
             - attribute_name: project

internal/surfer/testdata/apis/developerconnect/expected/current/surface/developerconnect/connections/_partials/_fetch_git_hub_installations_ga.yaml

@@ -31,6 +31,7 @@
           The resource name of the connection in the format
           `projects/*/locations/*/connections/*`.
         is_positional: false
+        is_primary_resource: false
         resource_spec:
           attributes:
             - attribute_name: project

internal/surfer/testdata/apis/developerconnect/expected/current/surface/developerconnect/connections/_partials/_fetch_linkable_git_repositories_ga.yaml

@@ -31,6 +31,7 @@
           The name of the Connection.
           Format: `projects/*/locations/*/connections/*`.
         is_positional: false
+        is_primary_resource: false
         resource_spec:
           attributes:
             - attribute_name: project