-
Couldn't load subscription status.
- Fork 28
Adds a copy action #150
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Adds a copy action #150
Conversation
1.x: the hopefully last two typos
|
Here is an experimental implementation, it was fairly straightforward :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I personally really like this. It's simple nomenclature and implementation.
Annecdotally to me the alternative approach of using runtime expressions is more powerful in some ways, but also limiting in others. We've been using them at Speakeasy to represent complex concepts in Arazzo but unfortunately we've had to build escape hatches for places where they don't work (specifically the lack of conditionals). I'd propose we accept this as a minor version bump, and do that as an independent addition to this change as part of future work given a set of differing use-cases.
|
cc: @RobertCraigie @yjp20 Would be great to get your feedback on this one. |
cd596cc to
cbac889
Compare
Signed-off-by: Vincent Biret <[email protected]>
Signed-off-by: Vincent Biret <[email protected]>
Signed-off-by: Vincent Biret <[email protected]>
Signed-off-by: Vincent Biret <[email protected]>
…e of the schema definition Signed-off-by: Vincent Biret <[email protected]>
Signed-off-by: Vincent Biret <[email protected]>
sync main to v1.1-dev
Signed-off-by: Vincent Biret <[email protected]>
Signed-off-by: Vincent Biret <[email protected]>
Signed-off-by: Vincent Biret <[email protected]>
Signed-off-by: Vincent Biret <[email protected]>
|
|
||
| The properties of the `update` object MUST be compatible with the target object referenced by the JSONPath key. When the Overlay document is applied, the properties in the `update` object are recursively merged with the properties in the target object with the same names; new properties are added to the target object. | ||
|
|
||
| The properties of the resolved `copy` object MUST be compatible with the target object referenced by the JSONPath key. When the Overlay document is applied, the properties in the resolved `copy` object are recursively merged with the properties in the target object with the same names; new properties are added to the target object. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
from the meeting, compatible is not explicitly defined, but this should be done as a separate PR, let's create a separate issue for that.
Signed-off-by: Vincent Biret <[email protected]>
Signed-off-by: Vincent Biret <[email protected]>
Signed-off-by: Vincent Biret <[email protected]>
|
@baywet — #justfyi I am working on an updated overlay.md to illustrate what I mentioned on the call today. I am adding some examples and changing some headings for consistency because with a table of examples the inconsistency stands out more. I also changed some text from passive to active voice because the ghost of my technical writing professor in university is always sitting on my shoulder commenting on my passive voice. Hope to have this available very soon, though I am not 100% sure how to submit a PR to a PR. Should I submit a PR to baywet/Overlay-Specification/feat/copy-node and then mention that I did so here? |
v1.1-dev: sync from main
Signed-off-by: Vincent Biret <[email protected]>
Signed-off-by: Vincent Biret <[email protected]>
Signed-off-by: Vincent Biret <[email protected]>
ci: sets up testing infra for 1.1 version
Signed-off-by: Vincent Biret <[email protected]>
|
For anybody following along, thanks for bearing with us while we clean up the branches/versions story. I've just "temporary aligned" things before retargeting to main (pending on #183). |
@mikeschinkel Thanks for doing this, yet a PR to my feature branch should be fine if it's a large set of changes, I just pushed a few new things since the call FYI. If the changes are smaller, suggestions (comments with suggestion) in this PR can work as well. |
Co-authored-by: Ralf Handl <[email protected]>
|
@baywet — I am getting my IDE flagging
|
|
@baywet — Here is the PR to your PR: baywet#1 I added a table listing the examples, and the rest of the changes followed from that to improve structure, clarity, and consistency (with no normative changes). Summary of changeds
All content above |
@mikeschinkel PR #118 contains a fix for that example. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@baywet Please re-target to main, if possible
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wrong folder, should be v1.1-dev.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should not be necessary
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops, we don’t want to rename a published spec. Maybe renaming to 1.1.0-dev.md will help?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wrong folder
fixes #146
fixes #121
closes #140
Hi everyone,
I've tried to get to the simplest common denominator, and incorporate some of the feedback that was already provided:
I'm proposing to add a copy field to the action, that's mutually exclusive with remove or update. It's functionally equivalent to the update field except that it uses a JSON path to source the JSON Node instead of defining it inline.
I know there was some additional thinking about templating expressions, sourcing from env variables etc... But I think those can be considered orthogonal and be added in a separate PR by their champions. I'd like to get the smallest possible unit that brings a meaningful contribution through.
As an image is better than a thousand words, here are a couple of examples.
Copying a node to a node that already exists
Here the existing foo path item properties are being copied to the bar path item.
Copying a node to a node that DOES NOT already exists
Moving an item
A couple of todos if that gets consensus: