Fix -pm <value> parsing and add comma-separated multi-fact -F (#14)

Two related query-ergonomics fixes surfaced while using the tool interactively. ## 1. `-pm <value>` failed with `unknown command "k8s"` pflag does not attach a space-separated value to a string flag (`-m`) grouped with a bool flag (`-p`), so `k8s` was left as a stray positional. Only `-pm=k8s` or the un-grouped `-p -m k8s` worked. - Allow one positional argument (`cobra.MaximumNArgs(1)`) and fall back to it for the match value when `-m` is empty (`matchValue()`). `-pm/-im/-ipm <value>` and a bare `-p <value>` now all work; `-m` still wins when both are given. ## 2. `-F a,b` (multiple facts) returned `{}` `-jF ipaddress,enc_role` queried a single fact literally named `ipaddress,enc_role`. - Split `-F` on commas (`splitFactNames`) and match any of them via an `or` over `["=","name",<n>]` clauses (`nameFilter`); a single name keeps the plain `=` form. - Key JSON output by each result's real fact name so all requested facts appear under the host. Both paths have unit tests; verified live: `node-lookup -R -pm externaldns | node-lookup -jF ipaddress,enc_role` returns both facts per host. AGENTS.md updated. Reviewed-on: #14 Co-authored-by: Ben Vincent <ben@unkin.net> Co-committed-by: Ben Vincent <ben@unkin.net>
2026-07-05 20:05:45 +10:00
parent e070357d3f
commit 5982d257d5
3 changed files with 220 additions and 31 deletions
@@ -9,6 +9,8 @@ import (
 	"path/filepath"
 	"strings"
 	"testing"
+
+	"gopkg.in/yaml.v3"
 )

 // ---- helpers ----------------------------------------------------------------
@@ -142,6 +144,101 @@ func TestBuildQuery_ValidJSON(t *testing.T) {
 	}
 }

+// ---- multi-fact -F (comma-separated) ----------------------------------------
+
+func TestSplitFactNames(t *testing.T) {
+	cases := map[string][]string{
+		"ipaddress":            {"ipaddress"},
+		"ipaddress,enc_role":   {"ipaddress", "enc_role"},
+		"ipaddress, enc_role ": {"ipaddress", "enc_role"}, // trims spaces
+		"a,,b,":                {"a", "b"},                // drops empties
+		"":                     nil,
+	}
+	for in, want := range cases {
+		got := splitFactNames(in)
+		if len(got) != len(want) {
+			t.Fatalf("splitFactNames(%q) = %v, want %v", in, got, want)
+		}
+		for i := range want {
+			if got[i] != want[i] {
+				t.Fatalf("splitFactNames(%q) = %v, want %v", in, got, want)
+			}
+		}
+	}
+}
+
+func TestBuildQuery_SingleFact_NoOr(t *testing.T) {
+	q := buildQuery("", "ipaddress", "", "enc_role", false, false, false)
+	if strings.Contains(q, `"or"`) {
+		t.Fatalf("single fact should not use 'or': %s", q)
+	}
+	if !strings.Contains(q, "ipaddress") {
+		t.Fatalf("expected fact name in query: %s", q)
+	}
+}
+
+func TestBuildQuery_MultiFact_UsesOr(t *testing.T) {
+	q := buildQuery("host1", "ipaddress,enc_role", "", "enc_role", false, false, false)
+	if !strings.Contains(q, `"or"`) {
+		t.Fatalf("expected 'or' over fact names: %s", q)
+	}
+	if !strings.Contains(q, "ipaddress") || !strings.Contains(q, "enc_role") {
+		t.Fatalf("expected both fact names: %s", q)
+	}
+	// Must remain valid PQL JSON, combined under "and" with the certname filter.
+	var v []interface{}
+	if err := json.Unmarshal([]byte(q), &v); err != nil {
+		t.Fatalf("query is not valid JSON: %v (%s)", err, q)
+	}
+	if v[0] != "and" {
+		t.Fatalf("expected top-level 'and', got %v", v[0])
+	}
+}
+
+func TestRun_JSON_MultipleFacts(t *testing.T) {
+	// Two facts returned for one host must both appear, keyed by their real name.
+	facts := []fact{
+		{Certname: "hosta", Name: "ipaddress", Value: rawJSON("198.18.0.1")},
+		{Certname: "hosta", Name: "enc_role", Value: rawJSON("roles::dns")},
+	}
+	out := runToString(t, facts, func(a *runArgs) {
+		a.showRole = false
+		a.factName = "ipaddress,enc_role"
+		a.jsonMode = true
+	})
+	var parsed map[string]map[string]interface{}
+	if err := json.Unmarshal([]byte(out), &parsed); err != nil {
+		t.Fatalf("output is not valid JSON: %v (%s)", err, out)
+	}
+	host := parsed["hosta"]
+	if host["ipaddress"] != "198.18.0.1" || host["enc_role"] != "roles::dns" {
+		t.Fatalf("expected both facts under host, got: %v", host)
+	}
+}
+
+// ---- matchValue (positional fallback for `-pm value`) -----------------------
+
+func TestMatchValue_FlagWins(t *testing.T) {
+	// An explicit -m value takes precedence over any positional arg.
+	if got := matchValue("flagval", []string{"posval"}); got != "flagval" {
+		t.Fatalf("expected flag value to win, got %q", got)
+	}
+}
+
+func TestMatchValue_PositionalFallback(t *testing.T) {
+	// This is the `-pm k8s` case: pflag leaves k8s as a positional because the
+	// grouped -m flag does not attach the space-separated value.
+	if got := matchValue("", []string{"k8s"}); got != "k8s" {
+		t.Fatalf("expected positional fallback, got %q", got)
+	}
+}
+
+func TestMatchValue_NoneGiven(t *testing.T) {
+	if got := matchValue("", nil); got != "" {
+		t.Fatalf("expected empty, got %q", got)
+	}
+}
+
 // ---- valueString / valueAny -------------------------------------------------

 func TestValueString_String(t *testing.T) {
@@ -677,6 +774,37 @@ func TestRun_Ansible(t *testing.T) {
 	if !strings.Contains(out, "hosta:") || !strings.Contains(out, "hostb:") {
 		t.Fatalf("expected both hosts in inventory, got: %q", out)
 	}
+	// The queried fact is attached as a host var.
+	if !strings.Contains(out, "enc_role: roles::db") || !strings.Contains(out, "enc_role: roles::web") {
+		t.Fatalf("expected fact host vars in inventory, got: %q", out)
+	}
+}
+
+func TestRun_Ansible_MultipleFacts(t *testing.T) {
+	// -F ipaddress,enc_role -A must include both facts as host vars.
+	facts := []fact{
+		{Certname: "hosta", Name: "ipaddress", Value: rawJSON("198.18.0.1")},
+		{Certname: "hosta", Name: "enc_role", Value: rawJSON("roles::dns")},
+	}
+	out := runToString(t, facts, func(a *runArgs) {
+		a.showRole = false
+		a.factName = "ipaddress,enc_role"
+		a.ansible = true
+	})
+
+	// Parse it back as YAML and assert the structure precisely.
+	var inv struct {
+		All struct {
+			Hosts map[string]map[string]interface{} `yaml:"hosts"`
+		} `yaml:"all"`
+	}
+	if err := yaml.Unmarshal([]byte(out), &inv); err != nil {
+		t.Fatalf("inventory is not valid YAML: %v (%s)", err, out)
+	}
+	host := inv.All.Hosts["hosta"]
+	if host["ipaddress"] != "198.18.0.1" || host["enc_role"] != "roles::dns" {
+		t.Fatalf("expected both facts as host vars, got: %v", host)
+	}
 }

 func TestRun_AllFacts_PrintsSortedByName(t *testing.T) {