Rename output.from and output.to

[fjtirado]

Output.from expression indicates the result of the task (and can be consumed by subsequent task)
Output.to expression indicates what should be the value of the context once the task is completed.

I think the names are confusing (it actually takes me a lot of time to understand why do we have two expressions in output), so I think we must change them.

output.from might be renamed to output.result (the expression indicates the result of the task)
output.to might be renamed to output.setContext (clearly states the purpose is to the set the value of the context), If we found that setContext break the single word paradigm that we are implicitly following in the DSL, then lets just use output.context.
Also we need to explicilly indicate in the documentation that it is assumed that the expression language should support merging context.
Related to that, if this assumption turns to be wrong, another possibility is to use

output: 
    context: 
        set:

Right now, context will only support set: but we leave the door open to add append:, delete: or merge: in future if needed

[cdavernas]

@JBBianchi @matthias-pichler-warrify WDYT?

1. Renaming output.from to output.result
2. Renaming output.to to output.context
3. Creating two expression properties to describe the operation to perform on the context (replace or update)

Personally:

1. Fine by me, though I personally like the actual wording. As demonstrated by Merge behaviour #837 and by @fjtirado admitedly late understanding by lack of info, it does however requires much more documentation.
2. Same than 1.
3. As expressed in Merge behaviour #837, I personally prefer it the way it is now, meaning that the user has the ability to write the expression he wants (i.e. replace or update the context). I do think however that the proposal makes sense, so I'll rally to the consensus on that one.

[JBBianchi]

Globally, more documentation won't hurt, for sure :)

If I can add my grain of salt, we might gain clarity by "splitting" output.from and output.to into distinct keywords. So my take would be the following:

1. Rename output.from to output, it shows the intention of the user to alter/transform the default output (but I have no strong opinion)
2. Rename output.to to export, it shows the intention of the user to export something from the task to a broader context (but again, I have no strong opinion)
3. Relying on the expression rather than making the spec more complex seems a better solution to me, so I'd rather keep this as they are.

[cdavernas]

1. Rename output.from to output, it shows the intention of the user to alter/transform the default output (but I have no strong opinion)
2. Rename output.to to export, it shows the intention of the user to export something from the task to a broader context (but again, I have no strong opinion)

You suggestion is awesome! Much clearer and ubiquitous!

This would however get rid of the critical output.schema property, as discussed in #837 😭
We could however move it elsewhere. Ideas?

[JBBianchi]

1. Rename output.from to output, it shows the intention of the user to alter/transform the default output (but I have no strong opinion)
2. Rename output.to to export, it shows the intention of the user to export something from the task to a broader context (but again, I have no strong opinion)

You suggestion is awesome! Much clearer and ubiquitous!

This would however get rid of the critical output.schema property, as discussed in #837 😭 We could however move it elsewhere. Ideas?

Good catch, I didn't think about that ... We could keep the to then maybe ? (or as)

input:
  from: ...
  schema: ...
output:
  to/as: ...
  schema: ...
export:
  to/as: ...
  schema: ...

[fjtirado]

The problem with to is that it might user though they can set the destination, which is not really true, so I vote for as rather than to.
I was going to suggest the same split than @JBBianchi ;)
Also, using export: as a separate keyword allow us to add in future set: or merge: if we change our mind.

[cdavernas]

Good catch, I didn't think about that ... We could keep the to then maybe ? (or as)

Works for me!

Even though I'm not opposed to using as, it doesn't, IMHO, carry the idea of a 'transformation', which is what the expression is used for in the context of the output.

On a side note, there is not way to specify the context schema, but I get its was a copy/paste leftover.

Should therefore be:

input:
  from: ...
  schema: ...
output:
  to/as: ...
  schema: ...
export: 'myexpression'

Or, if we believe we might need additional properties at some point, like @fjtirado said:

input:
  from: ...
  schema: ...
output:
  to/as: ...
  schema: ...
export: 
  to/as: .data += .mydata

[JBBianchi]

On a side note, there is not way to specify the context schema, but I get its was a copy/paste leftover.

It was intentional, why can't we support a schema for the context as well ? I think it's a point that's been raised, even in the previous version of the spec, that we don't know at some point in the workflow, what to expect in the context. With the schema, it give the opportunity to the user to defined what expected of the context to be at a given point in time.

[cdavernas]

@JBBianchi because you cannot forecast the state of the context (ex: if a switch state was executed before) at a point in time.

AFAIK, that never was brought up, also because you can both update the context (adding a new prop, removing one, etc) and replace it. For the replace it could work but has little to no value. For the update, that would be impossible or at least impractical (diff patch or replace), for little to no value.

IMO there's no way you, or any user, would like to document a whole schema just for updating a prop in a data bag (unordered, untyped).

[fjtirado]

@cdavernas I can foresee schema for context being used by users that want to restrict the context content (I do agree with you that the most frequent use case is "free" context ;)). As far as is optional I do not see any issue.

[JBBianchi]

@JBBianchi because you cannot forecast the state of the context (ex: if a switch state was executed before) at a point in time.

AFAIK, that never was brought up, also because you can both update the context (adding a new prop, removing one, etc) and replace it. For the replace it could work but has little to no value. For the update, that would be impossible or at least impractical (diff patch or replace), for little to no value.

IMO there's no way you, or any user, would like to document a whole schema just for updating a prop in a data bag (unordered, untyped).

I don't really agree with your arguments. Since the user defines the context, it is predictable. The user knows the types of operations being performed (merge, replace, patch, etc.) on the context. The only point I agree with is that writing the schemas can become cumbersome. It involves copying and pasting them, they might be complex, and most users won't bother. However, most users also won't utilize input/output schemas, at least not within the workflow. Nevertheless, I believe it is valuable to understand the structure of the data at any given point.

That being said, we can postpone this discussion for now. I believe we've reached a consensus on replacing output.to with export.as. (Personally, I would also prefer using output.as, but I don't have a strong preference. It could remain from, become to, or as - it's a minor detail.)

[fjtirado]

@JBBianchi I prefer output.as too (we need to document the purpose anyway, so its a minor thing)

[matthias-pichler-warrify]

The only point I agree with is that writing the schemas can become cumbersome. It involves copying and pasting them, they might be complex, and most users won't bother. However, most users also won't utilize input/output schemas, at least not within the workflow. Nevertheless, I believe it is valuable to understand the structure of the data at any given point.

It might be very valuable though to validate a subset of properties in the context. JSON Schema objects have additionalProperties: true by default so the schema

type: object
properties:
  foo:
    type: number

allows all the following values:

{
  "foo": 1
}

{
  "foo": 1,
  "bar": true
}

so it is trivial to write a context schema that for example only validates the exported properties 👍 It is not required to copy paste the schema to every task.

On another note the workflow schema also does not specify additionalProperties or unevaluatedProperties at the moment so arbitrary garbage properties can be set everywhere and it would still validate (I was thinking about opening a PR to fix this)

[ricardozanini]

+1 @matthias-pichler-warrify to fix the validation, hence I think we can also improve our CI to have invalid DSLs