Support input references by package (input type no longer required)#1605
Support input references by package (input type no longer required)#1605teresaromero merged 11 commits intoelastic:mainfrom
Conversation
Made-with: Cursor
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
Made-with: Cursor
mrodm
left a comment
There was a problem hiding this comment.
As here it does not run the validation from package-spec, would it be helpful to add the validation to ensure that Type or Package is set in the manifest (policy template)? and the Input or Package is set in Datastreams ?
package-registry/packages/package.go
Line 695 in 05720fb
package-registry/packages/datastream.go
Line 243 in 05720fb
packages/datastream.go
Outdated
There was a problem hiding this comment.
According to the changes in package-spec PR elastic/package-spec#1071, it looks like that under Stream it should also be added package
And it this struct, it should be one of the Input or Package mandatory
There was a problem hiding this comment.
I missed the datastream one! thanks. I've added the field to Streams and remove the validation tag at the Input. I was going through and did not see why input was missing the yaml tag at the struct, so i added too.
- Introduced `validStreamsInput` method in `DataStream` to validate stream inputs. - Added `validatePackageReference` method in `Package` to enforce XOR rule for policy template inputs. - Implemented unit tests for various package validation scenarios, including valid and invalid configurations. - Created test data packages to validate stream input rules and ensure proper error handling for invalid cases.
@mrodm i thought about it on the initial implementation, but i finally thought that validation is done when building the package, thanks to the package-spec validation. Also an argument was that this is special from a certain version of the spec... so not all the packages share this. However, i've added after your comment as i think there is no harm in double check this and make sure as we remove a requirement, the rule is well stablished. I've added tests with mock packages to validate the aproach, i wonder if package-spec should be a dependency here 🤔 as we are kind of duplicating the validations |
Co-authored-by: Mario Rodriguez Molins <marrodmo@gmail.com>
mrodm
left a comment
There was a problem hiding this comment.
LGTM
Just add some final comments to update changelog and test packages.
CHANGELOG.md
Outdated
|
|
||
| ### Added | ||
|
|
||
| * Support input references by package: input `type` is no longer required when `package` is set (format_version 3.6.0+, aligns with package-spec and composable packages). [#1605](https://github.com/elastic/package-registry/pull/1605) |
There was a problem hiding this comment.
Nit.
This should add a reference to the streams (input and package fields).
| @@ -0,0 +1,11 @@ | |||
| format_version: 3.0.0 | |||
There was a problem hiding this comment.
Nit. Would these packages require spec 3.6.0?
| format_version: 3.0.0 | |
| format_version: 3.6.0 |
| @@ -0,0 +1,20 @@ | |||
| format_version: 3.0.0 | |||
There was a problem hiding this comment.
Nit. spec 3.6.0 ?
| format_version: 3.0.0 | |
| format_version: 3.6.0 |
| @@ -0,0 +1,21 @@ | |||
| format_version: 3.0.0 | |||
There was a problem hiding this comment.
As mentioned in the other test packages.
| format_version: 3.0.0 | |
| format_version: 3.6.0 |
There was a problem hiding this comment.
As these test packages are used as part of the packages module, I'd move those to https://github.com/elastic/package-registry/tree/main/packages/testdata
There was a problem hiding this comment.
i've moved the success test cases to the package folder. i still want to test validation introduced with wrong packages
…put validation - Updated package paths in `package_test.go` to use relative paths. - Introduced new test data packages for policy templates and stream validation, including: - `policy_template_both_inputs` with both type and package set (invalid case). - `policy_template_type_input` with type only (valid case). - `stream_validation_invalid` with an invalid stream definition. - Added corresponding README files for documentation.
💚 Build Succeeded
History
|
What
Why
packageortypeis required for inputs in policy templates and data streams;typeis no longer required whenpackageis used.packageinstead oftype.Checklist
typeandpackagetypecontinue to workRelated