Skip to content

Feat: Structurelize patch object #587

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

Conversation

yue9944882
Copy link
Member

@yue9944882 yue9944882 commented Jun 6, 2019

@brendandburns we saw that many developers are trying to pass an object directly into the patch call, making this structurelized can prevent them from that. additionally, this pull moves the patch format consts into the custom.V1Patch, which makes it easier to find

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jun 6, 2019
@k8s-ci-robot k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jun 6, 2019
@yue9944882 yue9944882 force-pushed the feat/structureize-patch-object branch 3 times, most recently from 9a71f20 to 440d0ba Compare June 6, 2019 05:38
@brendandburns
Copy link
Contributor

Did you change the code generator to do this? We need to make sure we can re-generate the code going forward...

@yue9944882
Copy link
Member Author

@brendandburns yep the change in generator is submitted here. kubernetes-client/gen#118. it's basically adding a type mapping to the xml configuration, i noticed the python client has already done the mapping for V1Patch class.

@yue9944882
Copy link
Member Author

https://github.com/kubernetes/kubernetes/blob/3d4124f2e083591c98e8a874760011781d3ee15c/staging/src/k8s.io/apiserver/pkg/endpoints/installer.go#L737

the endpoint for patch api endpoint is defined w/ structurelized schema but the pre-process script in the generator somehow omitted that.

@yue9944882 yue9944882 force-pushed the feat/structureize-patch-object branch 3 times, most recently from 5ceafdf to 38f5da1 Compare June 11, 2019 07:03
@brendandburns
Copy link
Contributor

Cool. A couple of small comments and then I think this is good to go.

@yue9944882 yue9944882 force-pushed the feat/structureize-patch-object branch from 38f5da1 to 71aa54c Compare June 13, 2019 04:45
@yue9944882
Copy link
Member Author

lets get this in!

this.value = value;
}

private String value;
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: move this above the constructor.

throw new UnsupportedOperationException("deserializing patch data is not supported");
}
}

Copy link
Contributor

Choose a reason for hiding this comment

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

nit: delete empty line.

@brendandburns
Copy link
Contributor

Sorry, a couple of additional nits... I'm going to lgtm, but can you address in a follow-up PR?

Thanks!

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 13, 2019
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: brendandburns, yue9944882

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:
  • OWNERS [brendandburns,yue9944882]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot merged commit da58401 into kubernetes-client:master Jun 13, 2019
yue9944882 added a commit to yue9944882/java that referenced this pull request Jun 14, 2019
k8s-ci-robot added a commit that referenced this pull request Jun 15, 2019
…llow-up

Follow-up of #587: addressing minor comments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants