Conversation
|
I like the general direction. |
|
Feedback from the meeting from both @lornajane and @ralfhandl :
|
Signed-off-by: Vincent Biret <vincentbiret@hotmail.com>
Signed-off-by: Vincent Biret <vincentbiret@hotmail.com>
Signed-off-by: Vincent Biret <vincentbiret@hotmail.com>
Signed-off-by: Vincent Biret <vincentbiret@hotmail.com>
Signed-off-by: Vincent Biret <vincentbiret@hotmail.com>
Signed-off-by: Vincent Biret <vincentbiret@hotmail.com>
e709bde to
06a21c9
Compare
ralfhandl
left a comment
There was a problem hiding this comment.
Mostly wording and capitalization
Co-authored-by: Ralf Handl <ralf.handl@gmail.com>
Co-authored-by: Ralf Handl <ralf.handl@gmail.com>
Co-authored-by: Ralf Handl <ralf.handl@gmail.com>
Signed-off-by: Vincent Biret <vincentbiret@hotmail.com>
…rlay-Specification into feat/action-templates Signed-off-by: Vincent Biret <vincentbiret@hotmail.com>
Signed-off-by: Vincent Biret <vincentbiret@hotmail.com>
|
@ralfhandl @lornajane I pushed another update a couple of minutes ago. I wasn't happy about the whole reusable actions vs action templates kind of thing. After chatting with @mikekistler internally I realized we could simply define a an action template reference object as "you can override anything from the resolved template in the reference" like JSON schema does to some extent. And keep things extra simple. Let me know what you think! |
…ferences Signed-off-by: Vincent Biret <vincentbiret@hotmail.com>
Signed-off-by: Vincent Biret <vincentbiret@hotmail.com>
|
Update, I've had some more time to spend on this. I believe I've addressed all the feedback and got this to a place much closer to a finalized version/ready for a formal vote. Noteable changes:
Let me know if you have any additional comments or questions. |
Signed-off-by: Vincent Biret <vincentbiret@hotmail.com>
Signed-off-by: Vincent Biret <vincentbiret@hotmail.com>
Signed-off-by: Vincent Biret <vincentbiret@hotmail.com>
…r versions of the specification Signed-off-by: Vincent Biret <vincentbiret@hotmail.com>
|
Note: I've just added a lot of tests to improve the coverage. This is going to temporarily make the review more difficult. I've extracted the tests for the current schema (not the changes in this pull request) in #317 in the hope that we merge that one first, making the diff here smaller. |
Signed-off-by: Vincent Biret <vincentbiret@hotmail.com>
| | Field Name | Type | Description | | ||
| | ---- | :----: | ---- | | ||
| | <a name="reusable-action-parameter-name"></a>name | `string` | **REQUIRED** The name of the parameter, MUST match the key for the string literal replacement expression. | | ||
| | <a name="reusable-action-parameter-default>default | Any | A default value for the parameter to use when no value is provided by the reference. Optional. | |
There was a problem hiding this comment.
collecting notes from my implementation work: it might be better to restrain this to string instead of any json value. Having any json value poses a couple of issues:
- string interpolation is not "just string interpolation" anymore (you need to consider the object tree), and that brings a higher chance of ending up with an invalid resolved value in the end
- environment variables would never be able to support that.
So both in the name of simplicity and uniformity I suggest we change this, and the parameter value field to string types instead of json values.
|
|
||
| | Field Name | Type | Description | | ||
| | ---- | :----: | ---- | | ||
| | <a name="reusable-action-reference-ref"></a>$ref | `string` | **REQUIRED** A [same-document](https://www.rfc-editor.org/rfc/rfc3986.html#section-4.4) (or fragment-only) relative URI reference, per RFC3986 §4.4, and that the fragment syntax is JSON Pointer, with the pointer prefix restricted to `/components/actions/`. | |
There was a problem hiding this comment.
@handrews I think that also effectively means that any reusable action (entries under components/actions) MUST have / and ~ encoded according to the JSON pointer semantics. I think we should call this out explicitly in the components section, what do you think?
| | ---- | :----: | ---- | | ||
| | <a name="reusable-action-parameters"></a>parameters | [[Reusable action parameter object](#reusable-action-parameter-object)] | A list of parameters to be used during string literal replacement. Optional. | | ||
| | <a name="reusable-action-environment-variables"></a>environmentVariables | [[Reusable action parameter object](#reusable-action-parameter-object)] | A list of environment variables to be used during string literal replacement. Optional. | | ||
| | <a name="reusable-action-action-fields"></a>Any field defined in the [action object](#action-object) | mixed | The [string literal replacement syntax](#string-literal-replacement-syntax) MAY be used for any of the fields, and the replacements MUST be evaluated as the reusable action reference is being resolved. | |
There was a problem hiding this comment.
Here we need to restrict the interpolation syntax to apply only to string fields (Target, Copy, Description), because for others (Remove) it breaks the JSON schema validation of the document).
And for Update, it poses a lot of additional questions (env vs params, what happens if the resulting JSON doesn't match the structure we expect (requirement for this field), etc...
|
Finalized the implementation of the current version of the spec over here BinkyLabs/openapi-overlays-dotnet#276 (some LLMs were hurt in the process) |
ralfhandl
left a comment
There was a problem hiding this comment.
Only reviewed the examples.
Co-authored-by: Ralf Handl <ralf.handl@gmail.com>
Signed-off-by: Vincent Biret <vincentbiret@hotmail.com>
Signed-off-by: Vincent Biret <vincentbiret@hotmail.com>
mikekistler
left a comment
There was a problem hiding this comment.
I think this is very close now. I left a few more comments about possible improvements.
| | Field Name | Type | Description | | ||
| | ---- | :----: | ---- | | ||
| | <a name="reusable-action-parameters"></a>parameters | [[Reusable action parameter object](#reusable-action-parameter-object)] | A list of parameters to be used during string literal replacement. Optional. | | ||
| | <a name="reusable-action-environment-variables"></a>environmentVariables | [[Reusable action parameter object](#reusable-action-parameter-object)] | A list of environment variables to be used during string literal replacement. Optional. | |
There was a problem hiding this comment.
I wonder if environmentVariables should be defined under components rather than in each Reusable Action Object. Unlike parameters, the names of these variables are fixed in the enviroment. I suppose different actions could use different default values for an environment variable, but that seem very unlikely and a poor practice.
There was a problem hiding this comment.
I briefly considered that approach, and quickly realized that it's probably not the one that's going to get consensus:
- we've already tried root level "variables" in feat: adds a parameter object for the action object #238
- multiple levels references resolutions have an implementation cost and add to the cognitive load, the feedback has been overwhelmingly in favour of "keep things as simple as possible"
- there are benefits from having a consistent approach between parameters and environment variables. In my implementation, the same code handles both. The only difference being where the values come from. I suspect most implementers will appreciate that.
- not doing this now does not prevent us from adding support for "root level parameter definitions" in a future minor version. (a little similar to external references support) and we could base this on popular demand.
Let me know if you have any additional comments or questions.
versions/1.2.0-dev.md
Outdated
|
|
||
| #### Reusable Action Object | ||
|
|
||
| This object is a component that can be reused by Reusable Action Reference Objects, it represents one or more changes to be applied to the target document at the locations defined by the target JSONPath expression and reused in one of more action references. |
There was a problem hiding this comment.
I think this needs a little more wordsmithing. Right now it feels like two sentences joined by a comma rather than a period. But that is not quite right either, since it is the composition of a Reusable Action Object and Action Reference that "represents one or more changes ..."
There was a problem hiding this comment.
coerced the LLM into coming up with something more intelligible in c04856d, let me know what you think.
Signed-off-by: Vincent Biret <vincentbiret@hotmail.com>
Signed-off-by: Vincent Biret <vincentbiret@hotmail.com>
ralfhandl
left a comment
There was a problem hiding this comment.
Only minor suggestions
| @@ -0,0 +1,10 @@ | |||
| overlay: 1.2.0 | |||
| info: | |||
| title: Reusable action environment variables must be an array | |||
There was a problem hiding this comment.
Non-reusable action objects can't have environment variables
| actions: | ||
| - target: '$.info' | ||
| environmentVariables: | ||
| name: stageName |
There was a problem hiding this comment.
| name: stageName | |
| - name: stageName |
Make the forbidden construct syntactically valid
There was a problem hiding this comment.
This seems to be a duplicate of tests/v1.2-dev/fail/action-parameters-invalid.yaml.
|
|
||
| | Field Name | Type | Description | | ||
| | ---- | :----: | ---- | | ||
| | <a name="reusable-action-parameters"></a>parameters | [[Reusable action parameter object](#reusable-action-parameter-object)] | A list of parameters to be used during string literal replacement. Optional. | |
There was a problem hiding this comment.
| | <a name="reusable-action-parameters"></a>parameters | [[Reusable action parameter object](#reusable-action-parameter-object)] | A list of parameters to be used during string literal replacement. Optional. | | |
| | <a name="reusable-action-parameters"></a>parameters | [[Reusable Action Parameter Object](#reusable-action-parameter-object)] | A list of parameters to be used during string literal replacement. Optional. | |
| | Field Name | Type | Description | | ||
| | ---- | :----: | ---- | | ||
| | <a name="reusable-action-parameters"></a>parameters | [[Reusable action parameter object](#reusable-action-parameter-object)] | A list of parameters to be used during string literal replacement. Optional. | | ||
| | <a name="reusable-action-environment-variables"></a>environmentVariables | [[Reusable action parameter object](#reusable-action-parameter-object)] | A list of environment variables to be used during string literal replacement. Optional. | |
There was a problem hiding this comment.
| | <a name="reusable-action-environment-variables"></a>environmentVariables | [[Reusable action parameter object](#reusable-action-parameter-object)] | A list of environment variables to be used during string literal replacement. Optional. | | |
| | <a name="reusable-action-environment-variables"></a>environmentVariables | [[Reusable Action Parameter Object](#reusable-action-parameter-object)] | A list of environment variables to be used during string literal replacement. Optional. | |
| | ---- | :----: | ---- | | ||
| | <a name="reusable-action-parameters"></a>parameters | [[Reusable action parameter object](#reusable-action-parameter-object)] | A list of parameters to be used during string literal replacement. Optional. | | ||
| | <a name="reusable-action-environment-variables"></a>environmentVariables | [[Reusable action parameter object](#reusable-action-parameter-object)] | A list of environment variables to be used during string literal replacement. Optional. | | ||
| | <a name="reusable-action-action-fields"></a>Any field defined in the [action object](#action-object) | mixed | The [string literal replacement syntax](#string-literal-replacement-syntax) MAY be used for any of the fields, and the replacements MUST be evaluated as the reusable action reference is being resolved. | |
There was a problem hiding this comment.
| | <a name="reusable-action-action-fields"></a>Any field defined in the [action object](#action-object) | mixed | The [string literal replacement syntax](#string-literal-replacement-syntax) MAY be used for any of the fields, and the replacements MUST be evaluated as the reusable action reference is being resolved. | | |
| | <a name="reusable-action-action-fields"></a>Any field defined in the [Action Object](#action-object) | mixed | The [string literal replacement syntax](#string-literal-replacement-syntax) MAY be used for any of the fields, and the replacements MUST be evaluated as the reusable action reference is being resolved. | |
|
|
||
| A reusable action is similar to an action but differs in important ways: | ||
|
|
||
| 1. It may omit any action field, in particular the `target` field, as these may be supplied by the Reusable Action Reference Object. |
There was a problem hiding this comment.
| 1. It may omit any action field, in particular the `target` field, as these may be supplied by the Reusable Action Reference Object. | |
| 1. It may omit any action field, in particular the `target` field, as these may be supplied by the [Reusable Action Reference Object](#reusable-action-reference-object). |
| A reusable action is similar to an action but differs in important ways: | ||
|
|
||
| 1. It may omit any action field, in particular the `target` field, as these may be supplied by the Reusable Action Reference Object. | ||
| 1. Any action field in the Reusable Action may employ string interpolation using variables specified in the `parameters` or `environmentVariables` fields. |
There was a problem hiding this comment.
| 1. Any action field in the Reusable Action may employ string interpolation using variables specified in the `parameters` or `environmentVariables` fields. | |
| 1. Any action field in the Reusable Action Object may employ string interpolation using variables specified in the `parameters` or `environmentVariables` fields. |
|
|
||
| 1. It may omit any action field, in particular the `target` field, as these may be supplied by the Reusable Action Reference Object. | ||
| 1. Any action field in the Reusable Action may employ string interpolation using variables specified in the `parameters` or `environmentVariables` fields. | ||
| 1. A Reusable Action MUST be referenced by a Reusable Action Reference Object to have any effect on the OpenAPI description. |
There was a problem hiding this comment.
| 1. A Reusable Action MUST be referenced by a Reusable Action Reference Object to have any effect on the OpenAPI description. | |
| 1. A Reusable Action Object has to be referenced by a Reusable Action Reference Object to have any effect on the OpenAPI description. |
Don't use MUST as this might be misunderstood as "a Reusable Action Object MUST be referenced at least once".
| | ---- | :----: | ---- | | ||
| | <a name="reusable-action-reference-ref"></a>$ref | `string` | **REQUIRED** A [same-document](https://www.rfc-editor.org/rfc/rfc3986.html#section-4.4) (or fragment-only) relative URI reference, per RFC3986 §4.4, and that the fragment syntax is JSON Pointer, with the pointer prefix restricted to `/components/actions/`. | | ||
| | <a name="reusable-action-reference-parameter-values></a>parameterValues | `Map(string, Any)` | A set of values to use for the reusable action parameters, the key MUST match the parameter name. Optional. | | ||
| | <a name="reusable-action-reference-action-fields"></a>Any field defined in the [action object](#action-object) | mixed | Any field defined in the [Action Object](#action-object) to be used as an override to the value resolved in the reusable action. The [string literal replacement syntax](#string-literal-replacement-syntax) MAY NOT be used for any of the fields. Optional. | |
There was a problem hiding this comment.
| | <a name="reusable-action-reference-action-fields"></a>Any field defined in the [action object](#action-object) | mixed | Any field defined in the [Action Object](#action-object) to be used as an override to the value resolved in the reusable action. The [string literal replacement syntax](#string-literal-replacement-syntax) MAY NOT be used for any of the fields. Optional. | | |
| | <a name="reusable-action-reference-action-fields"></a>Any field defined in the [Action Object](#action-object) | mixed | Any field defined in the [Action Object](#action-object) to be used as an override to the value resolved in the reusable action. The [string literal replacement syntax](#string-literal-replacement-syntax) MAY NOT be used for any of the fields. Optional. | |
This pull request adds action templates.
fixes #33
fixes #136
fixes #270
closes #238
This is another attempt to solve a scale limitation in the current specification. Action templates are better than the previous parameters proposal because:
The pull request is incomplete as it is, it's a draft, I want to collect feedback on the approach before making any further investments.