Add accept_ignored_values to field spec#743
Conversation
jsoriano
left a comment
There was a problem hiding this comment.
If needed, I think we should make this a test config setting rather than part of the field definition.
|
@flash1293 Are we supporting this in the validation.yml or directly in the field definition in the fields.yml, ecs.yml files ? |
|
@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. |
|
@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 ? |
@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? |
|
@ShourieG I might misunderstand, but in order to extend the |
|
Hmm, maybe we could have something like errors:
exclude_checks:
- "SVR00006:error.message" # do not fail on error.message getting ignoredWhat 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.
The spec for test config files is barely validated, we accept almost anything there (due to this |
jsoriano
left a comment
There was a problem hiding this comment.
This approach LGTM, let's solve the issues with CI. Thanks!
| for the specified field names can't be indexed for any incoming documents. This | ||
| should only be used if the failure is related to the test environment | ||
| and wouldn't happen in production. Mitigate the issue via mapping or | ||
| pipeline changes otherwise. |
| @@ -1,2 +1,4 @@ | |||
| wait_for_data_timeout: 10m | |||
| skip_ignored_fields: | |||
| - error.message | |||
There was a problem hiding this comment.
Nit. Could we also add a good example in the good_v3 package?
|
As this change is not required for elastic/elastic-package#1738, could you include the implementation for this feature there before merging this? |
💚 Build Succeeded
History
|
jsoriano
left a comment
There was a problem hiding this comment.
Please consider adding the implementation for this in elastic/elastic-package#1738 before merging 🙏
Thanks!
|
elastic/elastic-package#1738 is merged, could you merge @jsoriano ? (I don't have the permission to do so) |
|
Merged, thanks! |
What does this PR do?
This PR adds a new property
skip_ignored_fieldsto the system test spec which is planned to be used in theelastic-packagesystem tests.Why is it important?
As discussed in elastic/elastic-package#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
test/packagesthat prove my change is effective.spec/changelog.yml.