diff --git a/README.md b/README.md index b7beb59..f2c1d9f 100644 --- a/README.md +++ b/README.md @@ -242,10 +242,13 @@ A missed host means an unvalidated catalog, so every ambiguity widens: `--explain`, in `unresolved_references` in `--json`, on stderr in plain mode, and set `summary.resolution_incomplete`. -A fact record that arrives with no certname belongs to no host and cannot be -folded into the estate table, so it is counted per fact and reported in -`node_warnings`, in `--explain` and on stderr, and sets -`summary.resolution_incomplete` too. +A fact record that arrives with no certname belongs to no host, and one whose +value does not decode carries nothing to fold; either way it cannot enter the +estate table, and a host whose records all fail leaves the table with them. Both +are reported in `node_warnings`, in `--explain` and on stderr, counted in +`summary.dropped_fact_records`, and set `summary.resolution_incomplete` too. If +the drops empty the table altogether the run fails, and the warnings and the +tally go to stderr with the error rather than dying with it. Paths outside the Puppet modulepath — `.woodpecker/`, `doc/`, `spec/`, `vendor/`, `Gemfile`, `*.md` — cannot reach a catalog and are skipped. diff --git a/cmd/catalog-diff/main.go b/cmd/catalog-diff/main.go index c56f85b..85f48d8 100644 --- a/cmd/catalog-diff/main.go +++ b/cmd/catalog-diff/main.go @@ -254,7 +254,7 @@ func run(ctx context.Context, out io.Writer, hostname string, opts *options) err server := puppetserver.New(opts.server, tlsCfg, opts.timeout) facts := puppetdb.New(opts.puppetdb, opts.timeout) - facts.Warn = func(msg string) { fmt.Fprintln(os.Stderr, "warning:", msg) } + facts.Warn = func(msg string, _ int) { fmt.Fprintln(os.Stderr, "warning:", msg) } healthCtx, cancel := context.WithTimeout(ctx, 30*time.Second) defer cancel() diff --git a/cmd/catalog-impact/main.go b/cmd/catalog-impact/main.go index bdf4adc..c74263a 100644 --- a/cmd/catalog-impact/main.go +++ b/cmd/catalog-impact/main.go @@ -67,7 +67,7 @@ func main() { SilenceUsage: true, SilenceErrors: true, RunE: func(cmd *cobra.Command, args []string) error { - return run(cmd.Context(), cmd.OutOrStdout(), opts) + return run(cmd.Context(), cmd.OutOrStdout(), cmd.ErrOrStderr(), opts) }, } @@ -89,7 +89,7 @@ func main() { os.Exit(exitOK) } -func run(ctx context.Context, out io.Writer, opts *options) error { +func run(ctx context.Context, out, errOut io.Writer, opts *options) error { if opts.jsonOut && opts.explain { return fmt.Errorf("--json and --explain are mutually exclusive") } @@ -119,12 +119,22 @@ func run(ctx context.Context, out io.Writer, opts *options) error { return fmt.Errorf("indexing %s: %w", opts.repo, err) } - nodes, nodeWarnings, err := impact.LoadNodes(ctx, puppetdb.New(opts.puppetdb, opts.timeout)) - if err != nil { - return err + // A dropped fact record can take a whole host out of the estate table, so + // it rides the same NodeWarnings channel as an unattributable one, through + // every output path and through a failed load. + facts := puppetdb.New(opts.puppetdb, opts.timeout) + var dropped []impact.NodeWarning + facts.Warn = func(msg string, records int) { + dropped = append(dropped, impact.NodeWarning{Message: msg, Records: records}) } - analyzer := &impact.Analyzer{Index: idx, Nodes: nodes, NodeWarnings: nodeWarnings} + nodes, nodeWarnings, err := impact.LoadNodes(ctx, facts) + warnings := append(dropped, nodeWarnings...) + if err != nil { + return loadFailed(errOut, warnings, err) + } + + analyzer := &impact.Analyzer{Index: idx, Nodes: nodes, NodeWarnings: warnings} result := analyzer.Analyze(opts.base, branch, files, opts.limit) switch { @@ -134,26 +144,41 @@ func run(ctx context.Context, out io.Writer, opts *options) error { return impact.RenderExplain(out, result) } - warn(result) + warn(errOut, result) return impact.RenderPlain(out, result) } // warn surfaces the widenings and dead ends on stderr so a piped host list // stays clean. -func warn(r *impact.Result) { +func warn(w io.Writer, r *impact.Result) { for _, file := range r.UnmappedFiles { - fmt.Fprintf(os.Stderr, "warning: %s matches no mapping rule; every host is included\n", file) + _, _ = fmt.Fprintf(w, "warning: %s matches no mapping rule; every host is included\n", file) } for _, line := range impact.UnresolvedLines(r.UnresolvedRefs) { - fmt.Fprintf(os.Stderr, "warning: %s did not resolve to a class name; the host list may be incomplete\n", line) - } - for _, line := range r.NodeWarnings { - fmt.Fprintf(os.Stderr, "warning: %s; the estate table may be missing hosts\n", line) + _, _ = fmt.Fprintf(w, "warning: %s did not resolve to a class name; the host list may be incomplete\n", line) } + warnNodes(w, r.NodeWarnings) for _, class := range r.OrphanClasses { - fmt.Fprintf(os.Stderr, "warning: class %s is declared by nothing and listed in no hiera_include; it reaches no hosts\n", class) + _, _ = fmt.Fprintf(w, "warning: class %s is declared by nothing and listed in no hiera_include; it reaches no hosts\n", class) } if r.Truncated { - fmt.Fprintf(os.Stderr, "warning: %d hosts affected, output capped at %d by --limit\n", r.Summary.Hosts, len(r.Hosts)) + _, _ = fmt.Fprintf(w, "warning: %d hosts affected, output capped at %d by --limit\n", r.Summary.Hosts, len(r.Hosts)) } } + +func warnNodes(w io.Writer, lines []string) { + for _, line := range lines { + _, _ = fmt.Fprintf(w, "warning: %s; the estate table may be missing hosts\n", line) + } +} + +// loadFailed keeps the warnings that explain a failed load from dying with it: +// no report is rendered once the run stops here, so they go to stderr in every +// output mode and the tally rides on the error. +func loadFailed(w io.Writer, warnings []impact.NodeWarning, err error) error { + warnNodes(w, impact.NodeWarningMessages(warnings)) + if n := impact.DroppedRecords(warnings); n > 0 { + return fmt.Errorf("%w (%d fact record(s) dropped)", err, n) + } + return err +} diff --git a/cmd/catalog-impact/main_test.go b/cmd/catalog-impact/main_test.go index 3723b19..3231901 100644 --- a/cmd/catalog-impact/main_test.go +++ b/cmd/catalog-impact/main_test.go @@ -3,6 +3,8 @@ package main import ( "bytes" "context" + "encoding/json" + "io" "net/http" "net/http/httptest" "strings" @@ -48,7 +50,7 @@ func TestRunWithExplicitFileList(t *testing.T) { puppetdb: fakePuppetDB(t), timeout: 5 * time.Second, } - if err := run(context.Background(), &out, opts); err != nil { + if err := run(context.Background(), &out, io.Discard, opts); err != nil { t.Fatalf("run: %v", err) } if got, want := out.String(), "web1.example.net\n"; got != want { @@ -66,7 +68,7 @@ func TestRunExplain(t *testing.T) { explain: true, timeout: 5 * time.Second, } - if err := run(context.Background(), &out, opts); err != nil { + if err := run(context.Background(), &out, io.Discard, opts); err != nil { t.Fatalf("run: %v", err) } if !strings.Contains(out.String(), "the whole estate is affected") { @@ -76,14 +78,158 @@ func TestRunExplain(t *testing.T) { func TestRunRejectsConflictingOutputFlags(t *testing.T) { opts := &options{jsonOut: true, explain: true} - if err := run(context.Background(), &bytes.Buffer{}, opts); err == nil { + if err := run(context.Background(), &bytes.Buffer{}, io.Discard, opts); err == nil { t.Fatal("expected an error for --json with --explain") } } func TestRunRejectsNegativeLimit(t *testing.T) { opts := &options{limit: -1} - if err := run(context.Background(), &bytes.Buffer{}, opts); err == nil { + if err := run(context.Background(), &bytes.Buffer{}, io.Discard, opts); err == nil { t.Fatal("expected an error for a negative --limit") } } + +// undecodableEstateBody strips the value from every one of web1's records, so +// each decodes to nothing and the host leaves the estate table with it. +const undecodableEstateBody = `[ + {"certname":"web1.example.net","name":"enc_role"}, + {"certname":"web1.example.net","name":"country"}, + {"certname":"web1.example.net","name":"os"}, + {"certname":"k8s1.example.net","name":"enc_role","value":"roles::infra::k8s::node"}, + {"certname":"k8s1.example.net","name":"country","value":"au"}, + {"certname":"k8s1.example.net","name":"os","value":{"name":"AlmaLinux","release":{"major":"9","minor":"8"}}} +]` + +func fakePuppetDBBody(t *testing.T, body string) string { + t.Helper() + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte(body)) + })) + t.Cleanup(srv.Close) + return srv.URL +} + +func droppedFactOptions(t *testing.T) *options { + t.Helper() + return &options{ + repo: "../../internal/impact/testdata/repo", + base: "origin/develop", + files: []string{"modules/jellyfin/manifests/install.pp"}, + puppetdb: fakePuppetDBBody(t, undecodableEstateBody), + timeout: 5 * time.Second, + } +} + +func TestRunReportsDroppedFactRecordsOnStderr(t *testing.T) { + var out, errOut bytes.Buffer + if err := run(context.Background(), &out, &errOut, droppedFactOptions(t)); err != nil { + t.Fatalf("run: %v", err) + } + if out.String() != "" { + t.Fatalf("web1 should have dropped out of the estate table, output = %q", out.String()) + } + for _, want := range []string{"skipped 3 undecodable fact(s)", "the estate table may be missing hosts"} { + if !strings.Contains(errOut.String(), want) { + t.Errorf("stderr = %q, want it to mention %q", errOut.String(), want) + } + } + if got := strings.Count(errOut.String(), "\n"); got != 1 { + t.Errorf("stderr has %d warning line(s), want 1: %q", got, errOut.String()) + } +} + +func TestRunReportsDroppedFactRecordsInJSON(t *testing.T) { + var out bytes.Buffer + opts := droppedFactOptions(t) + opts.jsonOut = true + if err := run(context.Background(), &out, io.Discard, opts); err != nil { + t.Fatalf("run: %v", err) + } + + var doc struct { + NodeWarnings []string `json:"node_warnings"` + Summary struct { + ResolutionIncomplete bool `json:"resolution_incomplete"` + DroppedFactRecords int `json:"dropped_fact_records"` + } `json:"summary"` + } + if err := json.Unmarshal(out.Bytes(), &doc); err != nil { + t.Fatalf("decoding --json output: %v", err) + } + if len(doc.NodeWarnings) != 1 { + t.Errorf("node_warnings = %v, want the one batched warning", doc.NodeWarnings) + } + if !doc.Summary.ResolutionIncomplete { + t.Error("resolution_incomplete is false after records were dropped") + } + if doc.Summary.DroppedFactRecords != 3 { + t.Errorf("summary.dropped_fact_records = %d, want 3", doc.Summary.DroppedFactRecords) + } +} + +func TestRunReportsDroppedFactRecordsInExplain(t *testing.T) { + var out bytes.Buffer + opts := droppedFactOptions(t) + opts.explain = true + if err := run(context.Background(), &out, io.Discard, opts); err != nil { + t.Fatalf("run: %v", err) + } + // web1 dropped three records in one warning, so the headline must count + // records, not messages. + for _, want := range []string{ + "fact records dropped", + "3 fact record(s) dropped", + "skipped 3 undecodable fact(s)", + } { + if !strings.Contains(out.String(), want) { + t.Errorf("explain output = %q, want it to mention %q", out.String(), want) + } + } +} + +// allUndecodableEstateBody strips the value from every record of every host, +// so the estate table comes back empty and the load fails. +const allUndecodableEstateBody = `[ + {"certname":"web1.example.net","name":"enc_role"}, + {"certname":"web1.example.net","name":"country"}, + {"certname":"k8s1.example.net","name":"enc_role"}, + {"certname":"k8s1.example.net","name":"country"} +]` + +func TestRunReportsDroppedFactRecordsWhenTheLoadFails(t *testing.T) { + for _, tc := range []struct { + name string + jsonOut bool + explain bool + }{ + {name: "plain"}, + {name: "json", jsonOut: true}, + {name: "explain", explain: true}, + } { + t.Run(tc.name, func(t *testing.T) { + var out, errOut bytes.Buffer + opts := &options{ + repo: "../../internal/impact/testdata/repo", + base: "origin/develop", + files: []string{"modules/jellyfin/manifests/install.pp"}, + puppetdb: fakePuppetDBBody(t, allUndecodableEstateBody), + jsonOut: tc.jsonOut, + explain: tc.explain, + timeout: 5 * time.Second, + } + + err := run(context.Background(), &out, &errOut, opts) + if err == nil { + t.Fatal("an empty estate table must fail the run") + } + if !strings.Contains(err.Error(), "4 fact record(s) dropped") { + t.Errorf("error = %q, want it to account for the 4 dropped records", err) + } + if !strings.Contains(errOut.String(), "skipped 4 undecodable fact(s)") { + t.Errorf("stderr = %q, want the dropped records reported", errOut.String()) + } + }) + } +} diff --git a/internal/impact/impact.go b/internal/impact/impact.go index 95ed0bb..8bc3179 100644 --- a/internal/impact/impact.go +++ b/internal/impact/impact.go @@ -27,6 +27,9 @@ type Summary struct { // statically, or a fact record could not be attributed to a host, so the // answer is a best effort rather than exact. ResolutionIncomplete bool `json:"resolution_incomplete"` + // DroppedFactRecords counts the fact records that never reached the estate + // table, which is not the number of warnings: one warning can cover many. + DroppedFactRecords int `json:"dropped_fact_records"` } // Result is the whole analysis, and the --json document. @@ -73,7 +76,7 @@ type Analyzer struct { Index *Index Nodes []Node // NodeWarnings carries whatever LoadNodes could not fold into Nodes. - NodeWarnings []string + NodeWarnings []NodeWarning } // Analyze maps changed files onto hosts. Files are processed in the order @@ -91,7 +94,7 @@ func (a *Analyzer) Analyze(base, branch string, files []string, limit int) *Resu Base: base, Branch: branch, ChangedFiles: uniqueSorted(files), - NodeWarnings: a.NodeWarnings, + NodeWarnings: NodeWarningMessages(a.NodeWarnings), TotalHosts: len(a.Nodes), } @@ -162,6 +165,7 @@ func (a *Analyzer) Analyze(base, branch string, files []string, limit int) *Resu Files: len(result.ChangedFiles), AllHostsTriggered: len(certnames) > 0 && len(certnames) == len(a.Nodes), ResolutionIncomplete: len(result.UnresolvedRefs) > 0 || len(result.NodeWarnings) > 0, + DroppedFactRecords: DroppedRecords(a.NodeWarnings), } if limit > 0 && len(certnames) > limit { diff --git a/internal/impact/impact_test.go b/internal/impact/impact_test.go index 8685f4d..cd1ebe5 100644 --- a/internal/impact/impact_test.go +++ b/internal/impact/impact_test.go @@ -275,9 +275,9 @@ func TestLoadNodesReportsRecordsWithNoCertname(t *testing.T) { if len(nodes) != 1 { t.Fatalf("nodes = %#v", nodes) } - want := []string{ - `fact "country": 1 record(s) had no certname and were dropped`, - `fact "enc_role": 2 record(s) had no certname and were dropped`, + want := []NodeWarning{ + {Message: `fact "country": 1 record(s) had no certname and were dropped`, Records: 1}, + {Message: `fact "enc_role": 2 record(s) had no certname and were dropped`, Records: 2}, } if !reflect.DeepEqual(warnings, want) { t.Errorf("warnings = %v, want %v", warnings, want) @@ -285,12 +285,36 @@ func TestLoadNodesReportsRecordsWithNoCertname(t *testing.T) { r := (&Analyzer{Index: testIndex(t), Nodes: nodes, NodeWarnings: warnings}). Analyze("base", "branch", []string{"README.md"}, 0) - if !reflect.DeepEqual(r.NodeWarnings, want) { - t.Errorf("Result.NodeWarnings = %v, want %v", r.NodeWarnings, want) + if !reflect.DeepEqual(r.NodeWarnings, NodeWarningMessages(want)) { + t.Errorf("Result.NodeWarnings = %v, want %v", r.NodeWarnings, NodeWarningMessages(want)) } if !r.Summary.ResolutionIncomplete { t.Error("a dropped fact record left the summary looking exact") } + // Three records in two messages: the summary counts records. + if r.Summary.DroppedFactRecords != 3 { + t.Errorf("Summary.DroppedFactRecords = %d, want 3", r.Summary.DroppedFactRecords) + } +} + +func TestLoadNodesReturnsWarningsWithItsError(t *testing.T) { + // Every record is unattributable, so the table is empty and the load + // fails. The warnings are the only explanation and must survive the error. + src := &stubFacts{records: []puppetdb.FactRecord{ + {Name: "enc_role", Value: "roles::base"}, + {Name: "enc_role", Value: "roles::web"}, + }} + + nodes, warnings, err := LoadNodes(context.Background(), src) + if err == nil { + t.Fatal("expected an error for an estate table with no attributable records") + } + if len(nodes) != 0 { + t.Fatalf("nodes = %#v", nodes) + } + if got := DroppedRecords(warnings); got != 2 { + t.Errorf("warnings account for %d dropped record(s), want 2", got) + } } func TestAnalyzeReportsFilesThatReachNoHost(t *testing.T) { diff --git a/internal/impact/node.go b/internal/impact/node.go index 7d68073..389e289 100644 --- a/internal/impact/node.go +++ b/internal/impact/node.go @@ -56,11 +56,40 @@ type FactSource interface { FactsByName(ctx context.Context, names []string) ([]puppetdb.FactRecord, error) } +// NodeWarning is one report of fact records that never reached the estate +// table, with the number of records it accounts for. One warning can stand for +// many records, so the count travels with the message rather than being +// inferred from it. +type NodeWarning struct { + Message string + Records int +} + +// NodeWarningMessages renders warnings one line each. +func NodeWarningMessages(warnings []NodeWarning) []string { + out := make([]string, 0, len(warnings)) + for _, w := range warnings { + out = append(out, w.Message) + } + return out +} + +// DroppedRecords totals the records the warnings account for. +func DroppedRecords(warnings []NodeWarning) int { + total := 0 + for _, w := range warnings { + total += w.Records + } + return total +} + // LoadNodes fetches FactNames for every node and folds them into a sorted // table. Nodes without an enc_role are kept: they still match country, os and // virtual selectors. A record with no certname belongs to no host and cannot // be kept, so it is returned as a warning rather than dropped in silence. -func LoadNodes(ctx context.Context, src FactSource) ([]Node, []string, error) { +// Warnings are returned alongside a failure too: they are usually what +// explains it. +func LoadNodes(ctx context.Context, src FactSource) ([]Node, []NodeWarning, error) { records, err := src.FactsByName(ctx, FactNames) if err != nil { return nil, nil, err @@ -93,15 +122,18 @@ func LoadNodes(ctx context.Context, src FactSource) ([]Node, []string, error) { } // anonymousWarnings renders the dropped-record tally one line per fact. -func anonymousWarnings(counts map[string]int) []string { +func anonymousWarnings(counts map[string]int) []NodeWarning { names := make([]string, 0, len(counts)) for name := range counts { names = append(names, name) } sort.Strings(names) - out := make([]string, 0, len(names)) + out := make([]NodeWarning, 0, len(names)) for _, name := range names { - out = append(out, fmt.Sprintf("fact %q: %d record(s) had no certname and were dropped", name, counts[name])) + out = append(out, NodeWarning{ + Message: fmt.Sprintf("fact %q: %d record(s) had no certname and were dropped", name, counts[name]), + Records: counts[name], + }) } return out } diff --git a/internal/impact/render.go b/internal/impact/render.go index 44aed7e..332ca83 100644 --- a/internal/impact/render.go +++ b/internal/impact/render.go @@ -63,8 +63,8 @@ func RenderExplain(w io.Writer, r *Result) error { } } if r.Summary.ResolutionIncomplete { - if err := p("resolution incomplete: %d class reference(s) could not be resolved statically", - len(r.UnresolvedRefs)); err != nil { + if err := p("resolution incomplete: %d class reference(s) unresolved, %d fact record(s) dropped", + len(r.UnresolvedRefs), r.Summary.DroppedFactRecords); err != nil { return err } } diff --git a/internal/puppetdb/puppetdb.go b/internal/puppetdb/puppetdb.go index 284fa0d..6cae2d5 100644 --- a/internal/puppetdb/puppetdb.go +++ b/internal/puppetdb/puppetdb.go @@ -12,6 +12,7 @@ import ( "io" "net/http" "net/url" + "os" "strings" "time" ) @@ -24,9 +25,13 @@ type Client struct { BaseURL string HTTP *http.Client - // Warn, when set, receives a message for every fact record that could not - // be decoded and was therefore dropped. - Warn func(string) + // Warn receives one message per batch of dropped fact records, with the + // number of records that batch accounts for — batches differ in size, so + // counting messages is not counting records. A nil Warn reports to stderr + // rather than discarding: a dropped record must never be silent. It is + // called on the goroutine that made the request, so a Client shared across + // goroutines needs a Warn that is safe to call concurrently. + Warn func(msg string, records int) } // New returns a client for baseURL. @@ -112,9 +117,9 @@ func (c *Client) FactsByName(ctx context.Context, names []string) ([]FactRecord, } out = append(out, FactRecord{Certname: r.Certname, Name: r.Name, Value: v}) } - if len(skipped) > 0 && c.Warn != nil { - c.Warn(fmt.Sprintf("skipped %d undecodable fact(s) in the estate fact table: %s", - len(skipped), strings.Join(skipped, ", "))) + if len(skipped) > 0 { + c.warn(len(skipped), "skipped %d undecodable fact(s) in the estate fact table: %s", + len(skipped), strings.Join(skipped, ", ")) } return out, nil } @@ -244,13 +249,24 @@ func (c *Client) fold(certname string, records []factRecord) (map[string]any, er if len(values) == 0 { return nil, fmt.Errorf("no usable facts for %s: all %d records were undecodable", certname, len(records)) } - if len(skipped) > 0 && c.Warn != nil { - c.Warn(fmt.Sprintf("skipped %d undecodable fact(s) for %s: %s", - len(skipped), certname, strings.Join(skipped, ", "))) + if len(skipped) > 0 { + c.warn(len(skipped), "skipped %d undecodable fact(s) for %s: %s", + len(skipped), certname, strings.Join(skipped, ", ")) } return values, nil } +// warn reports records dropped. An unwired hook falls back to stderr so that +// forgetting to set one loses visibility, never the record itself. +func (c *Client) warn(records int, format string, args ...any) { + msg := fmt.Sprintf(format, args...) + if c.Warn != nil { + c.Warn(msg, records) + return + } + fmt.Fprintln(os.Stderr, "warning:", msg) +} + func snippet(body []byte) string { s := strings.TrimSpace(string(body)) if len(s) > 200 { diff --git a/internal/puppetdb/puppetdb_test.go b/internal/puppetdb/puppetdb_test.go index 37224c4..72714ed 100644 --- a/internal/puppetdb/puppetdb_test.go +++ b/internal/puppetdb/puppetdb_test.go @@ -4,9 +4,12 @@ import ( "context" "encoding/json" "errors" + "io" "net/http" "net/http/httptest" + "os" "strings" + "sync" "testing" "time" ) @@ -288,8 +291,12 @@ func TestFactsSkipsUndecodableRecords(t *testing.T) { ]`) var warnings []string + var counted int c := New(srv.URL, 5*time.Second) - c.Warn = func(msg string) { warnings = append(warnings, msg) } + c.Warn = func(msg string, records int) { + warnings = append(warnings, msg) + counted += records + } facts, err := c.Facts(context.Background(), "host.example.net") if err != nil { @@ -302,6 +309,9 @@ func TestFactsSkipsUndecodableRecords(t *testing.T) { if len(warnings) != 1 || !strings.Contains(warnings[0], "broken") { t.Errorf("the skipped fact must be reported, got %v", warnings) } + if counted != 1 { + t.Errorf("Warn reported %d dropped record(s), want 1", counted) + } } func TestFactsWithNothingDecodableIsAnError(t *testing.T) { @@ -380,14 +390,20 @@ func TestFactsByNameSkipsUndecodableRecords(t *testing.T) { w.Header().Set("Content-Type", "application/json") _, _ = w.Write([]byte("\ufeff" + `[ {"certname":"a.example.net","name":"country","value":"au"}, - {"certname":"a.example.net","name":"broken"} + {"certname":"a.example.net","name":"broken"}, + {"certname":"b.example.net","name":"broken"}, + {"certname":"c.example.net","name":"broken"} ]`)) })) defer srv.Close() var warnings []string + var counted int c := New(srv.URL, 5*time.Second) - c.Warn = func(msg string) { warnings = append(warnings, msg) } + c.Warn = func(msg string, records int) { + warnings = append(warnings, msg) + counted += records + } records, err := c.FactsByName(context.Background(), []string{"country", "broken"}) if err != nil { @@ -396,7 +412,55 @@ func TestFactsByNameSkipsUndecodableRecords(t *testing.T) { if len(records) != 1 || records[0].Name != "country" { t.Fatalf("records = %#v", records) } + // One query batches its drops into one message, so the count has to come + // from the hook and not from the number of messages. if len(warnings) != 1 || !strings.Contains(warnings[0], "broken") { t.Errorf("the skipped fact must be reported, got %v", warnings) } + if counted != 3 { + t.Errorf("Warn reported %d dropped record(s), want 3", counted) + } +} + +func TestFactsByNameWithoutWarnHookReportsOnStderr(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte(`[{"certname":"a.example.net","name":"broken"}]`)) + })) + defer srv.Close() + + stderr := captureStderr(t) + if _, err := New(srv.URL, 5*time.Second).FactsByName(context.Background(), []string{"broken"}); err != nil { + t.Fatalf("FactsByName: %v", err) + } + if got := stderr(); !strings.Contains(got, "broken") { + t.Errorf("an unwired Warn must still report the drop, stderr = %q", got) + } +} + +// captureStderr redirects os.Stderr for the duration of the test and returns a +// func that restores it and yields what was written. +func captureStderr(t *testing.T) func() string { + t.Helper() + r, w, err := os.Pipe() + if err != nil { + t.Fatalf("pipe: %v", err) + } + saved := os.Stderr + os.Stderr = w + + var once sync.Once + var out string + read := func() string { + once.Do(func() { + os.Stderr = saved + _ = w.Close() + b, _ := io.ReadAll(r) + _ = r.Close() + out = string(b) + }) + return out + } + t.Cleanup(func() { read() }) + return read }