Skip to content
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

Strategic Merge Patch doc #535

Merged
merged 1 commit into from
Apr 19, 2017
Merged

Strategic Merge Patch doc #535

merged 1 commit into from
Apr 19, 2017

Conversation

mengqiy
Copy link
Member

@mengqiy mengqiy commented Apr 13, 2017

Pull Strategic Merge Patch out of API convention doc.
And refactor it.

Use the format what I discussed with @pwittrock offline.
More content need to be fill by @pwittrock in a followup PR.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Apr 13, 2017
@mengqiy
Copy link
Member Author

mengqiy commented Apr 13, 2017

/assign @pwittrock

Example usage in the patch:

```
$patch: replace
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Put the example in the context of an actual json struct. This doesn't show demonstrate how to create a patch with this directive.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nevermind, I should have kept reading :P

To delete items "b" and "c" from the original finalizers, the patch will be:

```yaml
$deleteFromPrimitiveList/finalizers:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would you add the context around this like you did for the others?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@pwittrock
Copy link
Member

Generally looks good. Just one comment about adding an example for the delete for primitives


### Purpose

`delete` directive indicates that the element that contains it should be deleted.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this apply to maps, or just lists? Will maps setting to null is how items are deleted right?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this apply to maps, or just lists?

Both map and list of maps.

Will maps setting to null is how items are deleted right?

That's one way. The other way is using delete directive in map.
I updated the doc to cover both.

live: null # set the value of the map key to null
```

Details of Strategic Merge Patch is in this [doc](../strategic-merge-patch.md).
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Details of Strategic Merge Patch is in this doc.

How about:

Details of Strategic Merge Patch are covered here.

@pwittrock
Copy link
Member

Looks good, one minor comment then ready to merge.

@mengqiy
Copy link
Member Author

mengqiy commented Apr 19, 2017

Addressed the comment. And squashed.

@pwittrock pwittrock merged commit f11408f into kubernetes:master Apr 19, 2017
@pwittrock
Copy link
Member

Thanks!

@mengqiy mengqiy deleted the SMP_doc branch April 19, 2017 20:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants