Skip to content

[system tests] Revisit multierrors when validating documents #2455

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Mar 6, 2025

Conversation

mrodm
Copy link
Contributor

@mrodm mrodm commented Mar 5, 2025

There are some scenarios that running system tests in packages could lead to duplicated errors like this one:

[0] parsing field value failed: [0] field "gcp.firewall.rule_details.ip_port_info.ip_protocol" is undefined
[1] field "gcp.firewall.rule_details.ip_port_info.port_range" is undefined
[1] parsing field value failed: [0] field "gcp.firewall.rule_details.ip_port_info.port_range" is undefined
[1] field "gcp.firewall.rule_details.ip_port_info.ip_protocol" is undefined

This can be observed when the gcp package is tested updating its format_version to be 3.0.1.

The errors shown to the user should be like:

[0] field "gcp.firewall.rule_details.ip_port_info.ip_protocol" is undefined
[1] field "gcp.firewall.rule_details.ip_port_info.port_range" is undefined

This PR revisits the usage of multierrors when validating the documents in system tests to show just unique errors.

How to test this PR locally

cd path/to/repo/integrations/

cd packages/gcp
# edit manifest and set format_version: 3.0.1

elastic-package stack up -v -d --version 9.1.0-SNAPSHOT

elastic-package test system -v --data-streams firewall

elastic-package stack down -v

@mrodm mrodm self-assigned this Mar 5, 2025
Comment on lines 1213 to +1216
if len(errs) == 0 {
return nil
}
return errs
return errs.Unique()
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This error variable comes from validateMapElement that returns a multierror.Error.

That's why here I updated all the functions calling this to return also multierror.Error (parseSingleElementValue and others)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove parsing field value failed: from the error messages

@elasticmachine
Copy link
Collaborator

💚 Build Succeeded

History

cc @mrodm

Comment on lines +713 to +715
errs := v.parseElementValue(key, *definition, val, doc)
if len(errs) > 0 {
return errs.Unique()
Copy link
Contributor Author

@mrodm mrodm Mar 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One of the main changes in this PR, here it is removed the parsing field value failed from the error (and just return all the errors that could be found).

To avoid errors in the output like:

[0] parsing field value failed: [0] field "gcp.firewall.rule_details.ip_port_info.ip_protocol" is undefined
[1] field "gcp.firewall.rule_details.ip_port_info.port_range" is undefined
[1] parsing field value failed: [0] field "gcp.firewall.rule_details.ip_port_info.port_range" is undefined
[1] field "gcp.firewall.rule_details.ip_port_info.ip_protocol" is undefined

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could those errors be reported without adding parsing field value failed?

Another example found:

[0] parsing field value failed: [0] parsing field value failed: field "cloudflare_logpush.email_security_alerts.attachments.Decrypted"'s Go type, bool, does not match the expected field type: keyword (field value: true)
[1] parsing field value failed: field "cloudflare_logpush.email_security_alerts.attachments.Encrypted"'s Go type, bool, does not match the expected field type: keyword (field value: true)

With the changes introduced in this PR, those errors would be written as:

[0] field "cloudflare_logpush.email_security_alerts.attachments.Decrypted"'s Go type, bool, does not match the expected field type: keyword (field value: true)
[1] field "cloudflare_logpush.email_security_alerts.attachments.Encrypted"'s Go type, bool, does not match the expected field type: keyword (field value: true)

@mrodm
Copy link
Contributor Author

mrodm commented Mar 5, 2025

test integrations

@elastic-vault-github-plugin-prod

Created or updated PR in integrations repository to test this version. Check elastic/integrations#12974

@mrodm mrodm marked this pull request as ready for review March 6, 2025 10:42
@mrodm mrodm requested a review from a team March 6, 2025 10:42
Copy link
Member

@jsoriano jsoriano left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, thanks!

@@ -672,14 +670,17 @@ func (v *Validator) validateMapElement(root string, elem common.MapStr, doc comm

err := v.validateScalarElement(key, val, doc)
if err != nil {
errs = append(errs, err)
errs = append(errs, err...)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@mrodm mrodm merged commit 1cb8aef into elastic:main Mar 6, 2025
3 checks passed
@mrodm mrodm deleted the revisit_multierrors branch March 6, 2025 12:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants