search

elastic / package-spec

EPR package specifications
Other
17 stars 70 forks source link

Add accept_ignored_values to field spec #743

Closed flash1293 closed 3 months ago

flash1293 commented 3 months ago

What does this PR do?

This PR adds a new property skip_ignored_fields to the system test spec which is planned to be used in the elastic-package system tests.

Why is it important?

As discussed in https://github.com/elastic/elastic-package/pull/1738 , it's sometimes important to mute errors during testing because they are not indicative of a real problem but related to the test setup.

This addition allows to override test failures similar to comments muting linting errors in source code.

Checklist

ShourieG commented 3 months ago

@flash1293 Are we supporting this in the validation.yml or directly in the field definition in the fields.yml, ecs.yml files ?

flash1293 commented 3 months ago

@ShourieG This was an old version, I just pushed a change, could you have a look? I added it to the system test definition now.

ShourieG commented 3 months ago

@flash1293 Having this tied to the package-spec forces us to increase the format_version, which in turn can cause un-intended errors while building with elastic-package, having all integrations using the latest format_version might have limitations, is there any other way we can include this, example adding it in the validations.yml file ?

flash1293 commented 3 months ago

is there any other way we can include this, example adding it in the validations.yml file

@ShourieG I think it should be a per-field setting, not a wholesale config to ignore all ignored fields in all tests as this would shadow newly introduced issues. Is there a way to do this without releasing a new package spec version?

ShourieG commented 3 months ago

is there any other way we can include this, example adding it in the validations.yml file

@ShourieG I think it should be a per-field setting, not a wholesale config to ignore all ignored fields in all tests as this would shadow newly introduced issues. Is there a way to do this without releasing a new package spec version?

@flash1293 Is the validations.yml file a valid approach here ? Since at the end of the day we are having to define the excluded fids for each integration test config, having it be a part of the validation file which is now supported by all existing integrations would be an easier trade-off?

flash1293 commented 3 months ago

@ShourieG I might misunderstand, but in order to extend the validation.yml to also take a list of fields to not check _ignored for, don't we need to release a version of the package spec as well? By my understanding currently validation.yml can only completely exclude certain checks, which is not what we want here

flash1293 commented 3 months ago

Hmm, maybe we could have something like SVR00006:<field name> would be an option (but it still seems like a workaround):

errors:
  exclude_checks:
   - "SVR00006:error.message" # do not fail on error.message getting ignored

What do you think @jsoriano ? To me it seems like the system test config is the right place for this, but I don't have a strong opinion really.

jsoriano commented 3 months ago

What do you think @jsoriano ? To me it seems like the system test config is the right place for this, but I don't have a strong opinion really.

Yeah, I prefer to have it in the test config.

Having this tied to the package-spec forces us to increase the format_version

The spec for test config files is barely validated, we accept almost anything there (due to this additionalProperties: true), so using this feature won't force to increase the format_version, at least as it is now being considered.

jsoriano commented 3 months ago

As this change is not required for https://github.com/elastic/elastic-package/pull/1738, could you include the implementation for this feature there before merging this? Thanks!

elasticmachine commented 3 months ago

:green_heart: Build Succeeded

History

flash1293 commented 3 months ago

https://github.com/elastic/elastic-package/pull/1738 is merged, could you merge @jsoriano ? (I don't have the permission to do so)

jsoriano commented 3 months ago

Merged, thanks!