Fix handling of global flags in scope definition

Previously the presence of global flags in a command like
"br --flag app a ent b sensor c" caused the CLI to run the command for
app, not sensor.

cli/api/catalog/catalog.go

@@ -24,7 +24,7 @@ import (
 	"github.com/apache/brooklyn-client/cli/models"
 	"github.com/apache/brooklyn-client/cli/net"
 	"net/url"
-        "path/filepath"
+	"path/filepath"
 	"errors"
 	"os"
 	"strings"

cli/commands/activity.go

@@ -50,7 +50,7 @@ func (cmd *Activity) Metadata() command_metadata.CommandMetadata {
 		Name:        "activity",
 		Aliases:     []string{"activities", "act", "acts"},
 		Description: "Show the activity for an application / entity",
-		Usage:       "BROOKLYN_NAME SCOPE activity [ ACTIVITYID]",
+		Usage:       "BROOKLYN_NAME SCOPE activity [ACTIVITYID]",
 		Flags: []cli.Flag{
 			cli.StringSliceFlag{
 				Name:  "children, c",

cli/scope/scope.go

@@ -89,49 +89,42 @@ var scopeSpecifier = map[string]func(scope *Scope, id string){
 // the function will return ([]string{"br", "doSomething"}, Scope{Application:1, Entity:2})
 func ScopeArguments(args []string) ([]string, Scope) {
 	scope := Scope{}
-
 	if len(args) < 2 {
 		return args, scope
 	}
-
-	command := args[0]
-	args = args[1:]
-
 	args = defineScope(args, &scope)
-
-	args = prepend(command, args)
-
 	return args, scope
 }
 
 func defineScope(args []string, scope *Scope) []string {
+	// args is: name [global flags] [scope and id pair] command [flags] [arguments]
+	// Skip name and global flags
+	idx := 1
+	for isFlag(args[idx]) {
+		idx += 1
+	}
+	newArgs := args[:idx]
+	args = args[idx:]
 
-	allScopesFound := false
-	for !allScopesFound && len(args) > 2 && args[1][0] != '-' {
-		if setAppropriateScope, nameOfAScope := scopeSpecifier[args[0]]; nameOfAScope {
-			setAppropriateScope(scope, args[1])
+	for len(args) > 2 && !isFlag(args[1]) {
+		if scopeFn, nameOfAScope := scopeSpecifier[args[0]]; nameOfAScope {
+			scopeFn(scope, args[1])
 			args = args[2:]
 		} else {
-			allScopesFound = true
+			break
 		}
 	}
-
 	setDefaultEntityIfRequired(scope)
+	return append(newArgs, args...)
+}
 
-	return args
+// true if the first character of arg is -
+func isFlag(arg string) bool {
+	return arg[0] == '-'
 }
 
 func setDefaultEntityIfRequired(scope *Scope) {
 	if "" == scope.Entity {
 		scope.Entity = scope.Application
 	}
 }
-
-func prepend(v string, args []string) []string {
-	result := make([]string, len(args)+1)
-	result[0] = v
-	for i, a := range args {
-		result[i+1] = a
-	}
-	return result
-}

cli/scope/scope_test.go

@@ -0,0 +1,92 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you 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 scope
+
+import (
+	"testing"
+)
+
+type testCase struct{
+	Args []string
+	ExpectedArgs []string
+	ExpectedScope Scope
+}
+
+func TestScope(t *testing.T) {
+	testCases := []testCase {
+		{
+			Args: []string{"br", "application"},
+			ExpectedArgs: []string{"br", "application"},
+			ExpectedScope: Scope{},
+		}, {
+			Args: []string{"br", "application", "appid", "entity"},
+			ExpectedArgs: []string{"br", "entity"},
+			ExpectedScope: Scope{Application: "appid", Entity: "appid"},
+		}, {
+			Args: []string{"br", "application", "appid", "entity", "entity"},
+			ExpectedArgs: []string{"br", "entity", "entity"},
+			// ScopeArguments sets Entity incorrectly when the command is entity.
+			ExpectedScope: Scope{Application: "appid", Entity: "appid"},
+		}, {
+			Args: []string{"br", "application", "appid", "entity", "entity", "sensor"},
+			ExpectedArgs: []string{"br", "sensor"},
+			ExpectedScope: Scope{Application: "appid", Entity: "entity"},
+		}, {
+			Args: []string{"br", "--verbose", "application", "appid", "entity", "entityId", "sensor"},
+			ExpectedArgs: []string{"br", "--verbose", "sensor"},
+			ExpectedScope: Scope{Application: "appid", Entity: "entityId"},
+		}, {
+			Args: []string{"br", "--verbose", "application", "appid", "entity", "entityId", "sensor", "http.port"},
+			ExpectedArgs: []string{"br", "--verbose", "sensor", "http.port"},
+			ExpectedScope: Scope{Application: "appid", Entity: "entityId"},
+		}, {
+			Args: []string{"br", "--verbose", "a", "appid", "e", "entityId", "v", "--children", "activityId"},
+			ExpectedArgs: []string{"br", "--verbose", "v", "--children", "activityId"},
+			ExpectedScope: Scope{Application: "appid", Entity: "entityId"},
+		},
+	}
+	for _, elem := range testCases {
+		argsOut, scope := ScopeArguments(elem.Args)
+		assertArgs(t, argsOut, elem.ExpectedArgs)
+		assertScope(t, scope, elem.ExpectedScope)
+	}
+}
+
+func assertArgs(t *testing.T, actual []string, expected []string) {
+	if len(actual) != len(expected) {
+		t.Errorf("%q != %q", actual, expected)
+		t.FailNow()
+	}
+	for idx, act := range actual {
+		exp := expected[idx]
+		if act != exp {
+			t.Errorf("mismatch at index %d: %q != %q", idx, actual, expected)
+		}
+	}
+}
+
+func assertScope(t *testing.T, actual Scope, expected Scope) {
+	if actual.Application != expected.Application ||
+		actual.Activity != expected.Activity ||
+		actual.Config != expected.Config ||
+		actual.Effector != expected.Effector ||
+		actual.Entity != expected.Entity {
+		t.Errorf("%v != %v", actual, expected)
+	}
+}