From 5982d257d5f259d101e9d5037fee20f61c7c7519 Mon Sep 17 00:00:00 2001 From: Ben Vincent Date: Sun, 5 Jul 2026 20:05:45 +1000 Subject: [PATCH] Fix -pm parsing and add comma-separated multi-fact -F (#14) Two related query-ergonomics fixes surfaced while using the tool interactively. ## 1. `-pm ` 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 ` and a bare `-p ` 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",]` 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: https://git.unkin.net/unkin/node-lookup/pulls/14 Co-authored-by: Ben Vincent Co-committed-by: Ben Vincent --- AGENTS.md | 18 +++++--- main.go | 105 ++++++++++++++++++++++++++++++++---------- main_test.go | 128 +++++++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 220 insertions(+), 31 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index cf90293..4cfa2ff 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -59,14 +59,16 @@ 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 -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) +./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 -./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 @@ -110,9 +112,11 @@ 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. -- **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. @@ -136,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 80ca754..623857f 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}) } @@ -236,6 +261,46 @@ 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 "" +} + +// 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)) @@ -306,25 +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 := factName - if 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 @@ -334,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}, @@ -389,8 +440,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,14 +455,14 @@ 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, } 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 a691d9b..b36af7a 100644 --- a/main_test.go +++ b/main_test.go @@ -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) {