From 8696097a6a18a5e7fef6e0777fce19c517f299f7 Mon Sep 17 00:00:00 2001 From: Ben Vincent Date: Sun, 5 Jul 2026 17:02:25 +1000 Subject: [PATCH 1/3] Accept match value positionally so -pm works pflag does not attach a space-separated value to a string flag that is grouped with a bool flag, so `node-lookup -R -pm k8s` parsed -p and left `k8s` as a stray positional, failing with "unknown command k8s". Only `-pm=k8s` or the un-grouped `-p -m k8s` worked, which is surprising. - Allow one positional argument (cobra.MaximumNArgs(1)) and fall back to it for the match value when -m is empty (matchValue()), so -pm/-im/-ipm and a bare `-p ` all work. -m still wins when both are given. - Add matchValue unit tests. - Document the positional value and the pflag grouping quirk in AGENTS.md. --- AGENTS.md | 10 ++++++---- main.go | 25 +++++++++++++++++++++++-- main_test.go | 23 +++++++++++++++++++++++ 3 files changed, 52 insertions(+), 6 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index cf90293..7f73dfa 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -59,10 +59,11 @@ installed. To load ad-hoc in the current shell, e.g. zsh: ./node-lookup -R # show all nodes with role fact ./node-lookup -n # lookup a specific node ./node-lookup -F # filter by fact name -./node-lookup -m # exact value match (-m) -./node-lookup -pm # partial/regex match (-p -m combined) -./node-lookup -im # inverse exact match (-i -m combined) -./node-lookup -ipm # inverse partial match (-i -p -m combined) +./node-lookup -R -m # exact value match (-m) +./node-lookup -R -pm # partial/regex match (-p -m combined) +./node-lookup -R -im # inverse exact match (-i -m combined) +./node-lookup -R -ipm # inverse partial match (-i -p -m combined) +./node-lookup -R -p # value may also be given positionally ./node-lookup -R -1 # node names only ./node-lookup -R -2 # values only ./node-lookup -R -C # count occurrences @@ -110,6 +111,7 @@ Show the active configuration (after all overrides applied): - **`loadConfig()`**: reads config file → applies env vars → returns `config` struct. Called once at startup in `main()`. - **`buildQuery()`**: returns a PuppetDB PQL-compatible JSON array string. Uses `roleFact` from config (not hardcoded). Match modifiers: `-p` (partial/regex, uses `~` op), `-i` (inverse, wraps with `not`), composable. +- **Match value / `matchValue()`**: the value to match comes from `-m/--match` or, if that is empty, an optional positional argument. The positional fallback exists because pflag does not attach a space-separated value to a string flag grouped with a bool flag, so in `-pm k8s` the `k8s` arrives as a positional. `-m` still wins when both are given. - **`queryPuppetDB(url, query)`**: takes the URL as a parameter — never reads globals. - **`processResults()`**: iterates facts, returns sorted `"certname value"` strings. JSON string values are unquoted; other JSON types rendered as compact JSON. - **Output modes**: JSON (`-j`), count (`-C`), Ansible YAML (`-A`), node-only (`-1`), value-only (`-2`), default (node + value). diff --git a/main.go b/main.go index 80ca754..b12419e 100644 --- a/main.go +++ b/main.go @@ -236,6 +236,21 @@ func stdinReader(f *os.File) (*bufio.Reader, bool) { return r, true } +// matchValue resolves the value to match against. The -m/--match flag wins; if +// it is empty, the (optional) positional argument is used instead. The +// positional fallback exists so combined shorthands like `-pm k8s` work — pflag +// leaves the space-separated `k8s` as a positional rather than attaching it to +// the grouped -m flag. +func matchValue(flagMatch string, args []string) string { + if flagMatch != "" { + return flagMatch + } + if len(args) > 0 { + return args[0] + } + return "" +} + func allFactsForNode(puppetDBURL, node string) ([]fact, error) { query, _ := json.Marshal([]interface{}{"=", "certname", node}) return queryPuppetDB(puppetDBURL, string(query)) @@ -389,8 +404,14 @@ func main() { ) rootCmd := &cobra.Command{ - Use: appName, + Use: appName + " [value]", Short: "Query PuppetDB for nodes.", + // Accept an optional positional match value in addition to -m. This makes + // combined shorthands like `-pm k8s` work: pflag does not attach a + // space-separated value to a string flag grouped with a bool flag (only + // `-pm=k8s` or `-p -m k8s` do), so `k8s` arrives here as a positional + // argument instead. Falling back to it keeps the ergonomic form working. + Args: cobra.MaximumNArgs(1), PersistentPreRunE: func(cmd *cobra.Command, args []string) error { if cmd.Flags().Changed("url") { cfg.PuppetDBURL = puppetDBURL @@ -398,7 +419,7 @@ func main() { return nil }, RunE: func(cmd *cobra.Command, args []string) error { - return run(cfg, nodeName, factName, match, showRole, partial, inverse, nodeOnly, valueOnly, count, ansible, jsonMode, allFacts) + return run(cfg, nodeName, factName, matchValue(match, args), showRole, partial, inverse, nodeOnly, valueOnly, count, ansible, jsonMode, allFacts) }, SilenceUsage: true, } diff --git a/main_test.go b/main_test.go index a691d9b..2cc8814 100644 --- a/main_test.go +++ b/main_test.go @@ -142,6 +142,29 @@ func TestBuildQuery_ValidJSON(t *testing.T) { } } +// ---- 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) { -- 2.47.3 From 103ebb2393b1c1049b78068c292bef91cb58fce0 Mon Sep 17 00:00:00 2001 From: Ben Vincent Date: Sun, 5 Jul 2026 17:16:07 +1000 Subject: [PATCH 2/3] Support comma-separated -F for querying multiple facts `node-lookup -jF ipaddress,enc_role` returned `{}` because it queried a single fact literally named "ipaddress,enc_role". Requesting several facts per host is a natural need (e.g. pairing an address with its role). - Split -F on commas (splitFactNames) and match any of them via an "or" over ["=","name",] 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 (previously keyed by the raw -F string). - Update the -F flag help and add unit tests (split, single vs multi query shape, multi-fact JSON). --- AGENTS.md | 4 ++- main.go | 46 +++++++++++++++++++++++++++------ main_test.go | 72 ++++++++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 113 insertions(+), 9 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 7f73dfa..8eada4b 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -59,6 +59,7 @@ installed. To load ad-hoc in the current shell, e.g. zsh: ./node-lookup -R # show all nodes with role fact ./node-lookup -n # lookup a specific node ./node-lookup -F # filter by fact name +./node-lookup -jF ipaddress,enc_role # several facts at once (comma-separated) ./node-lookup -R -m # exact value match (-m) ./node-lookup -R -pm # partial/regex match (-p -m combined) ./node-lookup -R -im # inverse exact match (-i -m combined) @@ -111,6 +112,7 @@ Show the active configuration (after all overrides applied): - **`loadConfig()`**: reads config file → applies env vars → returns `config` struct. Called once at startup in `main()`. - **`buildQuery()`**: returns a PuppetDB PQL-compatible JSON array string. Uses `roleFact` from config (not hardcoded). Match modifiers: `-p` (partial/regex, uses `~` op), `-i` (inverse, wraps with `not`), composable. +- **Multiple facts**: `-F` accepts a comma-separated list (`ipaddress,enc_role`). `splitFactNames()`/`nameFilter()` turn several names into an `or` over `["=","name",]` clauses; JSON output keys each value by the fact's real name so all requested facts appear per host. - **Match value / `matchValue()`**: the value to match comes from `-m/--match` or, if that is empty, an optional positional argument. The positional fallback exists because pflag does not attach a space-separated value to a string flag grouped with a bool flag, so in `-pm k8s` the `k8s` arrives as a positional. `-m` still wins when both are given. - **`queryPuppetDB(url, query)`**: takes the URL as a parameter — never reads globals. - **`processResults()`**: iterates facts, returns sorted `"certname value"` strings. JSON string values are unquoted; other JSON types rendered as compact JSON. @@ -138,5 +140,5 @@ mode (default, `-1`, `-2`, `-C`, `-j`, `-A`, `-a`). PuppetDB is stubbed with - `-1`, `-2`, `-C`, and `-A` all require `-R` or `-F`; the tool exits with an error otherwise. - `-C` (count) with stdin reads all lines as pre-fetched `"node value"` output for counting — it does **not** query PuppetDB per line. -- JSON output (`-j`) builds `{ hostname: { factname: value } }` where the fact key is the `-F` value, the `role_fact` config value (if `-R`), or `"value"` as fallback. +- JSON output (`-j`) builds `{ hostname: { factname: value } }` keyed by each result's actual fact name (so `-F ipaddress,enc_role` yields both per host); it falls back to the `-F` value, the `role_fact` config value (if `-R`), or `"value"` only when a result carries no name. - `config init` fails if the config file already exists (will not overwrite). diff --git a/main.go b/main.go index b12419e..e1cdd98 100644 --- a/main.go +++ b/main.go @@ -110,6 +110,31 @@ type fact struct { Value json.RawMessage `json:"value"` } +// splitFactNames splits a comma-separated -F value into trimmed, non-empty +// names, so `-F ipaddress,enc_role` queries both facts. +func splitFactNames(factName string) []string { + var names []string + for _, n := range strings.Split(factName, ",") { + if n = strings.TrimSpace(n); n != "" { + names = append(names, n) + } + } + return names +} + +// nameFilter returns a PQL filter matching any of the given fact names: a plain +// equality for one name, an "or" over per-name equalities for several. +func nameFilter(names []string) []interface{} { + if len(names) == 1 { + return []interface{}{"=", "name", names[0]} + } + or := []interface{}{"or"} + for _, n := range names { + or = append(or, []interface{}{"=", "name", n}) + } + return or +} + func buildQuery(node, factName, match, roleFact string, showRole, partial, inverse bool) string { type filter = []interface{} var filters []filter @@ -117,8 +142,8 @@ func buildQuery(node, factName, match, roleFact string, showRole, partial, inver if node != "" { filters = append(filters, filter{"=", "certname", node}) } - if factName != "" { - filters = append(filters, filter{"=", "name", factName}) + if names := splitFactNames(factName); len(names) > 0 { + filters = append(filters, nameFilter(names)) } else if showRole { filters = append(filters, filter{"=", "name", roleFact}) } @@ -326,12 +351,17 @@ func run(cfg config, nodeName, factName, match string, showRole, partial, invers if _, ok := hostFactMap[f.Certname]; !ok { hostFactMap[f.Certname] = map[string]interface{}{} } - key := factName + // Key by the fact's actual name so multiple -F facts (e.g. + // ipaddress,enc_role) each appear under the host. Fall back to the + // computed key only if the result carries no name. + key := f.Name if key == "" { - if showRole { - key = cfg.RoleFact - } else { - key = "value" + if key = factName; key == "" { + if showRole { + key = cfg.RoleFact + } else { + key = "value" + } } } hostFactMap[f.Certname][key] = valueAny(f.Value) @@ -426,7 +456,7 @@ func main() { f := rootCmd.Flags() f.StringVarP(&nodeName, "node", "n", "", "Node name") - f.StringVarP(&factName, "fact", "F", "", "Fact name") + f.StringVarP(&factName, "fact", "F", "", "Fact name (comma-separated for several, e.g. -F ipaddress,enc_role)") f.BoolVarP(&showRole, "role", "R", false, "Show role fact ("+defaultRoleFact+" by default)") f.StringVarP(&match, "match", "m", "", "Value to match (use with -p and/or -i)") f.BoolVarP(&partial, "partial", "p", false, "Partial/regex match modifier (combine with -m)") diff --git a/main_test.go b/main_test.go index 2cc8814..125b643 100644 --- a/main_test.go +++ b/main_test.go @@ -142,6 +142,78 @@ 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) { -- 2.47.3 From 5d0f69483bad1920b41ca7882a16a2b0d362dafe Mon Sep 17 00:00:00 2001 From: Ben Vincent Date: Sun, 5 Jul 2026 17:39:53 +1000 Subject: [PATCH 3/3] Include queried facts as host vars in -A inventory `-A` emitted hosts with empty vars, so `-F ipaddress,enc_role -A` lost the facts it just queried. Ansible inventories are far more useful with the values inline. - Extract factsByHost() (the {host: {fact: value}} builder) and share it between -j and -A so the Ansible inventory attaches each host's queried fact(s) as host vars, keyed by real fact name. - Strengthen the Ansible test to assert host vars and add a multi-fact case. --- AGENTS.md | 4 ++-- main.go | 54 +++++++++++++++++++++++++++++----------------------- main_test.go | 33 ++++++++++++++++++++++++++++++++ 3 files changed, 65 insertions(+), 26 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 8eada4b..4cfa2ff 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -68,7 +68,7 @@ installed. To load ad-hoc in the current shell, e.g. zsh: ./node-lookup -R -1 # node names only ./node-lookup -R -2 # values only ./node-lookup -R -C # count occurrences -./node-lookup -R -A # output as Ansible YAML inventory +./node-lookup -R -A # output as Ansible YAML inventory (queried facts become host vars) ./node-lookup -j # output as JSON { host → { fact → value } } ./node-lookup --url http://host:8080/... # override PuppetDB URL for this invocation echo -e "node1\nnode2" | ./node-lookup -R # pipe node names via stdin @@ -116,7 +116,7 @@ Show the active configuration (after all overrides applied): - **Match value / `matchValue()`**: the value to match comes from `-m/--match` or, if that is empty, an optional positional argument. The positional fallback exists because pflag does not attach a space-separated value to a string flag grouped with a bool flag, so in `-pm k8s` the `k8s` arrives as a positional. `-m` still wins when both are given. - **`queryPuppetDB(url, query)`**: takes the URL as a parameter — never reads globals. - **`processResults()`**: iterates facts, returns sorted `"certname value"` strings. JSON string values are unquoted; other JSON types rendered as compact JSON. -- **Output modes**: JSON (`-j`), count (`-C`), Ansible YAML (`-A`), node-only (`-1`), value-only (`-2`), default (node + value). +- **Output modes**: JSON (`-j`), count (`-C`), Ansible YAML (`-A`), node-only (`-1`), value-only (`-2`), default (node + value). `-j` and `-A` share `factsByHost()`, so both attach the queried fact(s) per host — as an object under the host (`-j`) or as inventory host vars (`-A`). - **Stdin support**: `stdinReader()` reads node names from stdin only when it is a real pipe/redirect carrying data (and no `-n` given). Terminals, `/dev/null`, and empty/closed pipes fall through to a normal query — so running without a TTY (e.g. invoked by an agent or CI) behaves like an interactive run instead of consuming empty input. - **SIGPIPE handling**: `signal.Ignore(syscall.SIGPIPE)` so pipes to `head` etc. work cleanly. diff --git a/main.go b/main.go index e1cdd98..623857f 100644 --- a/main.go +++ b/main.go @@ -276,6 +276,31 @@ func matchValue(flagMatch string, args []string) string { return "" } +// factsByHost groups collected facts into {certname: {factname: value}}. Each +// value is keyed by the fact's real name, so multiple -F facts each appear +// under the host; it falls back to the -F string, the role fact (with -R), or +// "value" only when a result carries no name. Shared by the -j and -A outputs. +func factsByHost(collected []fact, factName, roleFact string, showRole bool) map[string]map[string]interface{} { + out := map[string]map[string]interface{}{} + for _, f := range collected { + if _, ok := out[f.Certname]; !ok { + out[f.Certname] = map[string]interface{}{} + } + key := f.Name + if key == "" { + if key = factName; key == "" { + if showRole { + key = roleFact + } else { + key = "value" + } + } + } + out[f.Certname][key] = valueAny(f.Value) + } + return out +} + func allFactsForNode(puppetDBURL, node string) ([]fact, error) { query, _ := json.Marshal([]interface{}{"=", "certname", node}) return queryPuppetDB(puppetDBURL, string(query)) @@ -346,30 +371,10 @@ func run(cfg config, nodeName, factName, match string, showRole, partial, invers switch { case jsonMode: - hostFactMap := map[string]map[string]interface{}{} - for _, f := range collected { - if _, ok := hostFactMap[f.Certname]; !ok { - hostFactMap[f.Certname] = map[string]interface{}{} - } - // Key by the fact's actual name so multiple -F facts (e.g. - // ipaddress,enc_role) each appear under the host. Fall back to the - // computed key only if the result carries no name. - key := f.Name - if key == "" { - if key = factName; key == "" { - if showRole { - key = cfg.RoleFact - } else { - key = "value" - } - } - } - hostFactMap[f.Certname][key] = valueAny(f.Value) - } enc := json.NewEncoder(os.Stdout) enc.SetIndent("", " ") enc.SetEscapeHTML(false) - _ = enc.Encode(hostFactMap) + _ = enc.Encode(factsByHost(collected, factName, cfg.RoleFact, showRole)) case count: values := stdinLines @@ -379,10 +384,11 @@ func run(cfg config, nodeName, factName, match string, showRole, partial, invers fmt.Println(strings.Join(countResults(values), "\n")) case ansible: + // Attach each host's queried fact(s) as inventory host vars, e.g. + // `-F ipaddress,enc_role -A` yields hosts with ipaddress + enc_role set. hosts := map[string]interface{}{} - for _, line := range returnData { - host := strings.Fields(line)[0] - hosts[host] = map[string]interface{}{} + for host, vars := range factsByHost(collected, factName, cfg.RoleFact, showRole) { + hosts[host] = vars } inventory := map[string]interface{}{ "all": map[string]interface{}{"hosts": hosts}, diff --git a/main_test.go b/main_test.go index 125b643..b36af7a 100644 --- a/main_test.go +++ b/main_test.go @@ -9,6 +9,8 @@ import ( "path/filepath" "strings" "testing" + + "gopkg.in/yaml.v3" ) // ---- helpers ---------------------------------------------------------------- @@ -772,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) { -- 2.47.3