Enforce validation for taskfile field in includes

[semihbkgr]

fixes #1881

This makes the taskfile field within include required, disallowing null (unspecified) or empty values, even if inlined.

I’d like to clarify two points, @pd93:

1- Would it be preferable to return a TaskfileDecodeError in the UnmarshalYAML function when parsing the yaml into Include? This approach could make the error message more descriptive.

However, I aligned this error with the existing one we return when the Taskfile is not existing ("no such file or directory"), which is why the error is returned after the parsing step.

2- Should we differentiate between null and empty string values, or should we treat them the same and return an error for both?

[pd93]

1- Would it be preferable to return a TaskfileDecodeError in the UnmarshalYAML function when parsing the yaml into Include? This approach could make the error message more descriptive.

However, I aligned this error with the existing one we return when the Taskfile is not existing ("no such file or directory"), which is why the error is returned after the parsing step.

The distinction here is whether or not taskfile: is required in the schema or whether it is valid to be empty in the schema, but causes an error when processing.

Opinion: I think we should error in the UnmarshalYAML method and return a TaskfileDecodeError and we should update the schema.json to mark taskfile: as a required field when using an include: object. I'm not sure if jsonschema can specify non-empty on a scalar value? but if it can, we should do that too. @andreynering @vmaerten feel free to weigh-in.

2- Should we differentiate between null and empty string values, or should we treat them the same and return an error for both?

I can't see a situation where an empty string is useful.

[semihbkgr]

@pd93 We can use "minLength": 1 in the json schema to ensure the taskfile value is not empty.

[vmaerten]

I totally agree with you @pd93 !

[vmaerten]

Hey @semihbkgr,
when you have time, can you rebase so we can move forward on this ? 🙁

[semihbkgr]

Hi @vmaerten,
I’ve resolved the conflict, FYI.

[semihbkgr]

The null check is placed before decoding the valueNode because the Decode method from the yaml package does not invoke the UnmarshalYAML method if the node is null. This ensures that null values are caught early and handled with a proper error message, as the UnmarshalYAML logic would be skipped for such nodes.