From 1c21b00aae80a638a00ae5f549fcbcca32890660 Mon Sep 17 00:00:00 2001 From: "mergify[bot]" <37929162+mergify[bot]@users.noreply.github.com> Date: Mon, 4 Dec 2023 17:29:58 +0100 Subject: [PATCH] Add custom MarshalYAML() method to component to fix YAML marshaling of error values (#3835) (#3856) * add custom MarshalYAML to component * add changelog * add nil check * update tests --------- Co-authored-by: Pierre HILBERT (cherry picked from commit 93f159ac9c84c47a0517d59cd1d1e389f9dc85bd) Co-authored-by: Alex K <8418476+fearful-symmetry@users.noreply.github.com> Co-authored-by: Pierre HILBERT --- ...315-custom-yaml-marshal-for-component.yaml | 32 +++++++++++++++++++ .../coordinator/diagnostics_test.go | 4 +-- pkg/component/component.go | 13 +++++++- pkg/component/component_test.go | 25 +++++++++++++++ 4 files changed, 70 insertions(+), 4 deletions(-) create mode 100644 changelog/fragments/1701208315-custom-yaml-marshal-for-component.yaml diff --git a/changelog/fragments/1701208315-custom-yaml-marshal-for-component.yaml b/changelog/fragments/1701208315-custom-yaml-marshal-for-component.yaml new file mode 100644 index 00000000000..d6c6834ced5 --- /dev/null +++ b/changelog/fragments/1701208315-custom-yaml-marshal-for-component.yaml @@ -0,0 +1,32 @@ +# Kind can be one of: +# - breaking-change: a change to previously-documented behavior +# - deprecation: functionality that is being removed in a later release +# - bug-fix: fixes a problem in a previous version +# - enhancement: extends functionality but does not break or fix existing behavior +# - feature: new functionality +# - known-issue: problems that we are aware of in a given version +# - security: impacts on the security of a product or a user’s deployment. +# - upgrade: important information for someone upgrading from a prior version +# - other: does not fit into any of the other categories +kind: bug-fix + +# Change summary; a 80ish characters long description of the change. +summary: custom-yaml-marshal-for-component + +# Long description; in case the summary is not enough to describe the change +# this field accommodate a description without length limits. +# NOTE: This field will be rendered only for breaking-change and known-issue kinds at the moment. +description: Create a custom MarshalYAML() method to properly handle error fields + +# Affected component; usually one of "elastic-agent", "fleet-server", "filebeat", "metricbeat", "auditbeat", "all", etc. +component: component + +# PR URL; optional; the PR number that added the changeset. +# If not present is automatically filled by the tooling finding the PR where this changelog fragment has been added. +# NOTE: the tooling supports backports, so it's able to fill the original PR number instead of the backport PR number. +# Please provide it if you are adding a fragment for a different PR. +pr: https://github.com/elastic/elastic-agent/pull/3835 + +# Issue URL; optional; the GitHub issue related to this changeset (either closes or is part of). +# If not present is automatically filled by the tooling with the issue linked to the PR number. +issue: https://github.com/elastic/elastic-agent/issues/2940 diff --git a/internal/pkg/agent/application/coordinator/diagnostics_test.go b/internal/pkg/agent/application/coordinator/diagnostics_test.go index c6f2dc4fc97..ca5dcf6dc57 100644 --- a/internal/pkg/agent/application/coordinator/diagnostics_test.go +++ b/internal/pkg/agent/application/coordinator/diagnostics_test.go @@ -381,12 +381,10 @@ func TestDiagnosticComponentsActual(t *testing.T) { }, } - // The error values here shouldn't really be empty, this is a known bug, see - // https://github.com/elastic/elastic-agent/issues/2940 expected := ` components: - id: component-1 - error: {} + error: "component error" input_type: "test-input" output_type: "test-output" units: diff --git a/pkg/component/component.go b/pkg/component/component.go index 18060b645fd..d9100a6dd98 100644 --- a/pkg/component/component.go +++ b/pkg/component/component.go @@ -156,7 +156,11 @@ type Component struct { // Err used when there is an error with running this input. Used by the runtime to alert // the reason that all of these units are failed. - Err error `yaml:"error,omitempty"` + Err error `yaml:"-"` + // the YAML marshaller won't handle `error` values, since they don't implement MarshalYAML() + // the Component's own MarshalYAML method needs to handle this, and place any error values here instead of `Err`, + // so they can properly be rendered as a string + ErrMsg string `yaml:"error,omitempty"` // InputSpec on how the input should run. (not set when ShipperSpec set) InputSpec *InputRuntimeSpec `yaml:"input_spec,omitempty"` @@ -187,6 +191,13 @@ type Component struct { ShipperRef *ShipperReference `yaml:"shipper,omitempty"` } +func (c Component) MarshalYAML() (interface{}, error) { + if c.Err != nil { + c.ErrMsg = c.Err.Error() + } + return c, nil +} + // Type returns the type of the component. func (c *Component) Type() string { if c.InputSpec != nil { diff --git a/pkg/component/component_test.go b/pkg/component/component_test.go index 424f3a93147..d91b8e17452 100644 --- a/pkg/component/component_test.go +++ b/pkg/component/component_test.go @@ -36,6 +36,31 @@ import ( "github.com/stretchr/testify/require" ) +// fake error type used for the test below +type testErr struct { + data string +} + +func (t testErr) Error() string { + return t.data +} + +func TestComponentMarshalError(t *testing.T) { + testComponent := Component{ + ID: "test-device", + Err: testErr{data: "test error value"}, + } + componentConfigs := []Component{testComponent} + + outData, err := yaml.Marshal(struct { + Components []Component `yaml:"components"` + }{ + Components: componentConfigs, + }) + require.NoError(t, err) + require.Contains(t, string(outData), "test error value") +} + func TestToComponents(t *testing.T) { linuxAMD64Platform := PlatformDetail{ Platform: Platform{