Merge pull request #251 from cybozu-go/fix-sabactl-machines-get-with-…

…serials

Fix "GET machines" API

CHANGELOG.md

@@ -5,6 +5,12 @@ This project adheres to [Semantic Versioning](http://semver.org/).
 
 ## [Unreleased]
 
+### Fixed
+- Clarify spec of queries in "GET machines" API (#251)
+- Fix "GET machines" API for multiple serials (#251)
+- **Incompatible change**: Fix "without-labels" query of "GET machines" API for clarified spec (#251)
+
+
 ## [2.12.0] - 2022-04-18
 
 ### Changed

docs/api.md

@@ -228,16 +228,35 @@ $ curl -s -X POST 'localhost:10080/api/v1/machines' -d '
 
 Search registered machines. A user can specify the following URL queries.
 
-| Query                    | Description                             |
-| ------------------------ | --------------------------------------- |
-| `serial=<serial>`        | The serial number of the machine        |
-| `labels=<key=value>,...` | The labels of the machine.              |
-| `rack=<rack>`            | The rack number where the machine is in |
-| `role=<role>`            | The role of the machine                 |
-| `ipv4=<ip address>`      | IPv4 address                            |
-| `ipv6=<ip address>`      | IPv6 address                            |
-| `bmc-type=<bmc-type>`    | BMC type                                |
-| `state=<state>`          | The state of the machine                |
+| Query                     | Description                             |
+| ------------------------- | --------------------------------------- |
+| `serial=<serial>,...`     | The serial number of the machine        |
+| `labels=<key=value>,...`  | The labels of the machine.              |
+| `rack=<rack>,...`         | The rack number where the machine is in |
+| `role=<role>,...`         | The role of the machine                 |
+| `ipv4=<ip address>,...`   | IPv4 address                            |
+| `ipv6=<ip address>,...`   | IPv6 address                            |
+| `bmc-type=<bmc-type>,...` | BMC type                                |
+| `state=<state>,...`       | The state of the machine                |
+
+Note that the comma `,` should be encoded as `%2C` and the equals sign `=` of `<key=value>` in the value for the `labels` field should be encoded as `%3D`.
+These encodings are not shown explicitly in the examples.
+
+The comma-separated query values are interpreted as follows.
+
+* For `labels`, the values are interpreted as a sequence of ANDs.
+  E.g. `labels=A=a,B=b` filters machines so that each of the returned machines has labels of `A=a` *and* `B=b`.
+* For the other fields, the values are interpreted as a sequence of ORs.
+  E.g. `serial=AAA,BBB` filters machines so that each of the returned machines has a serial of `AAA` *or* `BBB`.
+
+Each query name can be prefixed with `without-`.
+This prefix negates the condition.
+
+Multiple query parameters are interpreted as a sequence of ANDs.
+E.g. `serial=AAA,BBB&without-rack=1` filters machines so that each of the returned machines has a serial of `AAA` or `BBB` *and* is not placed in the rack #1.
+
+You cannot give multiple values to a single field in the style of `field=value1&field=value2`.
+The result is undefined for that type of query.
 
 **Successful response**
 

docs/sabactl.md

@@ -74,14 +74,23 @@ Show machines filtered by query parameters.
 
 ```console
 $ sabactl machines get \
-    [--serial <serial>] \
-    [--rack <rack>] \
-    [--role <role>] \
+    [--serial <serial>,...] \
+    [--rack <rack>,...] \
+    [--role <role>,...] \
     [--labels <key=value>,...]
-    [--ipv4 <ip address>] \
-    [--ipv6 <ip address>] \
-    [--bmc-type <BMC type>] \
-    [--state <state>]
+    [--ipv4 <ip address>,...] \
+    [--ipv6 <ip address>,...] \
+    [--bmc-type <BMC type>,...] \
+    [--state <state>,...] \
+    [--without-serial <serial>,...] \
+    [--without-rack <rack>,...] \
+    [--without-role <role>,...] \
+    [--without-labels <key=value>,...]
+    [--without-ipv4 <ip address>,...] \
+    [--without-ipv6 <ip address>,...] \
+    [--without-bmc-type <BMC type>,...] \
+    [--without-state <state>,...] \
+    [--output json|simple]
 ```
 
 Detailed specification of the query parameters and the output JSON content is same as those of the [`GET /api/v1/machines` API](api.md#getmachines).

models/etcd/machine.go

@@ -269,7 +269,7 @@ func (d *driver) machineQuery(ctx context.Context, q sabakan.Query) ([]*sabakan.
 			serials[i] = string(kv.Key[len(KeyMachines):])
 		}
 	case len(q.Serial()) > 0:
-		serials = []string{q.Serial()}
+		serials = strings.Split(q.Serial(), ",")
 	default:
 		serials = d.mi.query(q)
 	}
@@ -292,7 +292,11 @@ func (d *driver) machineQuery(ctx context.Context, q sabakan.Query) ([]*sabakan.
 			return nil, err
 		}
 
-		if q.Match(m) {
+		matched, err := q.Match(m)
+		if err != nil {
+			return nil, err
+		}
+		if matched {
 			res = append(res, m)
 		}
 	}

models/etcd/machine_test.go

@@ -122,16 +122,27 @@ func testQuery(t *testing.T) {
 		t.Fatal(err)
 	}
 
-	q := sabakan.Query{"serial": "12345678"}
+	q := sabakan.Query{"serial": "12345678,12345679"}
 	resp, err := d.machineQuery(context.Background(), q)
 	if err != nil {
 		t.Fatal(err)
 	}
-	if len(resp) != 1 {
+	if len(resp) != 2 {
 		t.Fatalf("unexpected query result: %#v", resp)
 	}
-	if !q.Match(resp[0]) {
-		t.Errorf("unexpected responsed machine: %#v", resp[0])
+	matched, err := q.Match(resp[0])
+	if err != nil {
+		t.Fatal(err)
+	}
+	if !matched {
+		t.Errorf("unexpected machine in response: %#v", resp[0])
+	}
+	matched, err = q.Match(resp[1])
+	if err != nil {
+		t.Fatal(err)
+	}
+	if !matched {
+		t.Errorf("unexpected machine in response: %#v", resp[1])
 	}
 
 	q = sabakan.Query{"labels": "product=R630"}
@@ -142,8 +153,19 @@ func testQuery(t *testing.T) {
 	if len(resp) != 2 {
 		t.Fatalf("unexpected query result: %#v", resp)
 	}
-	if !(q.Match(resp[0]) && q.Match(resp[1])) {
-		t.Errorf("unexpected responsed machine: %#v", resp)
+	matched, err = q.Match(resp[0])
+	if err != nil {
+		t.Fatal(err)
+	}
+	if !matched {
+		t.Errorf("unexpected machine in response: %#v", resp[0])
+	}
+	matched, err = q.Match(resp[1])
+	if err != nil {
+		t.Fatal(err)
+	}
+	if !matched {
+		t.Errorf("unexpected machine in response: %#v", resp[1])
 	}
 
 	q = sabakan.Query{}

models/etcd/main_test.go

@@ -117,7 +117,8 @@ func initializeTestData(d *driver, ch <-chan struct{}) ([]*sabakan.Machine, erro
 			Role:       "worker",
 			RetireDate: time.Date(2018, time.November, 22, 1, 2, 3, 0, time.UTC),
 		}),
-		sabakan.NewMachine(sabakan.MachineSpec{Serial: "123456789",
+		sabakan.NewMachine(sabakan.MachineSpec{
+			Serial: "123456789",
 			Labels: map[string]string{"product": "R730"},
 			Role:   "worker",
 		}),

models/mock/machine.go

@@ -97,7 +97,11 @@ func (d *driver) machineQuery(ctx context.Context, q sabakan.Query) ([]*sabakan.
 
 	res := make([]*sabakan.Machine, 0)
 	for _, m := range d.machines {
-		if q.Match(m) {
+		matched, err := q.Match(m)
+		if err != nil {
+			return nil, err
+		}
+		if matched {
 			res = append(res, m)
 		}
 	}

pkg/sabactl/cmd/machines.go

@@ -16,6 +16,7 @@ import (
 
 var (
 	machinesGetParams  = make(map[string]*string)
+	machinesGetOutput  string
 	machinesCreateFile string
 )
 
@@ -32,11 +33,10 @@ var machinesGetCmd = &cobra.Command{
 	Long:  `Get machines from sabakan.`,
 
 	RunE: func(cmd *cobra.Command, args []string) error {
-		params := make(map[string]string)
-		outputFormat, ok := machinesGetParams["output"]
-		if ok {
-			delete(machinesGetParams, "output")
+		if machinesGetOutput != "json" && machinesGetOutput != "simple" {
+			return fmt.Errorf("unknown output format %q", machinesGetOutput)
 		}
+		params := make(map[string]string)
 		for k, v := range machinesGetParams {
 			params[k] = *v
 		}
@@ -45,7 +45,7 @@ var machinesGetCmd = &cobra.Command{
 			if err != nil {
 				return err
 			}
-			if ok && *outputFormat == "simple" {
+			if machinesGetOutput == "simple" {
 				w := tabwriter.NewWriter(cmd.OutOrStdout(), 0, 1, 1, ' ', 0)
 				w.Write([]byte("Serial\tRack\tRole\tState\tIPv4\tBMC\n"))
 				for _, m := range ms {
@@ -225,17 +225,13 @@ func init() {
 		"without-ipv6":     "without IPv6 address",
 		"without-bmc-type": "without BMC type",
 		"without-state":    "without State",
-		"output":           "Output format",
 	}
 	for k, v := range getOpts {
 		val := new(string)
 		machinesGetParams[k] = val
-		if k == "output" {
-			machinesGetCmd.Flags().StringVarP(val, k, "o", "", v)
-		} else {
-			machinesGetCmd.Flags().StringVar(val, k, "", v)
-		}
+		machinesGetCmd.Flags().StringVar(val, k, "", v)
 	}
+	machinesGetCmd.Flags().StringVarP(&machinesGetOutput, "output", "o", "json", "Output format [json,simple]")
 	machinesCreateCmd.Flags().StringVarP(&machinesCreateFile, "file", "f", "", "machiens in json")
 	machinesCreateCmd.MarkFlagRequired("file")