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

Add patchMergeKey and patchStrategy support to OpenAPI #44121

Merged
merged 3 commits into from
Apr 8, 2017

Conversation

mbohlool
Copy link
Contributor

@mbohlool mbohlool commented Apr 6, 2017

Support generating Open API extensions for strategic merge patch tags in go struct tags
Support patchStrategy and patchMergeKey.
Also support checking if the Open API extension and struct tags match.

Support generating Open API extensions for strategic merge patch tags in go struct tags

cc: @pwittrock @ymqytw

(Description mostly copied from #43833)

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

This change is Reviewable

@k8s-github-robot k8s-github-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. release-note-label-needed labels Apr 6, 2017
@mbohlool mbohlool added release-note-none Denotes a PR that doesn't merit a release note. and removed release-note-label-needed labels Apr 6, 2017
if len(extensions) == 0 {
return nil
}
g.Do("VendorExtensible: spec.VendorExtensible{\nExtensions: spec.Extensions{\n", nil)
Copy link
Member

Choose a reason for hiding this comment

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

This is not identical to before. If no extensions exist, this line should not present it the open api spec, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

right, I am checking that the line before. if len(extensions) is zero, then we just return without generating anything.

Copy link
Member

Choose a reason for hiding this comment

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

Oh yeah. You are right.

@k8s-github-robot k8s-github-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Apr 6, 2017
@mbohlool mbohlool force-pushed the patch branch 2 times, most recently from eaafb90 to fbbac5d Compare April 7, 2017 00:43
@mengqiy
Copy link
Member

mengqiy commented Apr 7, 2017

@k8s-bot verify test this

@mbohlool mbohlool added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-none Denotes a PR that doesn't merit a release note. labels Apr 7, 2017
@mengqiy
Copy link
Member

mengqiy commented Apr 7, 2017

LGTM, thanks!

@mbohlool mbohlool added this to the v1.7 milestone Apr 7, 2017
@mbohlool
Copy link
Contributor Author

mbohlool commented Apr 7, 2017

Needs approval from @wojtek-t or @lavalamp

Setting P1 because the PR needs to be merged before any API change or we risk endless rebases.

tagExtensionPrefix = "x-kubernetes-"
tagPatchStrategy = "patchStrategy"
tagPatchMergeKey = "patchMergeKey"
patchStrategyExtensionKey = "x-kubernetes-patch-strategy"
Copy link
Member

@lavalamp lavalamp Apr 7, 2017

Choose a reason for hiding this comment

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

Would it make sense to write this and below as tagExtensionPrefix + "patch-strategy"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was doing it that way, but because we had patch-strategy and patchStrategy both, I though it could be mixed up and I am not sure about the naming of only "patch-strategy". Something like patchStrategyPartialExtensionKey? :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed!

return "", nil
}
if len(tags) > 1 {
return "", fmt.Errorf("Multiple values is not allowed for tag %s", tag)
Copy link
Member

Choose a reason for hiding this comment

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

optional nit: s/is/are

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed!

return nil
}

// TODO(#44005): Move this validation outside of this generator (probably to policy verifier
Copy link
Member

Choose a reason for hiding this comment

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

nit: )

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed!

if err != nil {
return err
}
if patchMergeKeyStructTag != patchMergeKeyCommentTag {
Copy link
Member

Choose a reason for hiding this comment

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

It should be ok for it to not be set in the struct tag, correct? It's just that if both are present they must match?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it is a good question @pwittrock and @ymqytw should know the answer too :)

Copy link
Member

Choose a reason for hiding this comment

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

IMO we want always enforce they match, at least for now.
Otherwise, if the client move to use openapi someday and the server is still using the struct tags and they are inconsistent. Patch will do the wrong things.

@@ -89,6 +89,8 @@ message ClusterSpec {
// This is to help clients reach servers in the most network-efficient way possible.
// Clients can use the appropriate server address as per the CIDR that they match.
// In case of multiple matches, clients should use the longest matching CIDR.
// +patchMergeKey=clientCIDR
// +patchStrategy=merge
Copy link
Member

Choose a reason for hiding this comment

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

Should these get copied here? I guess it doesn't hurt anything. The tags should really be stripped out by the proto generator, I guess that can be fixed in another PR.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, looks like this isn't making it worse.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ack

@lavalamp
Copy link
Member

lavalamp commented Apr 7, 2017

/approve

nits can be fixed in a followup.

@k8s-github-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: lavalamp, mbohlool

Needs approval from an approver in each of these OWNERS Files:

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

@k8s-github-robot k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 7, 2017
@mbohlool mbohlool added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 7, 2017
@mengqiy
Copy link
Member

mengqiy commented Apr 8, 2017

@k8s-bot gci gce e2e test this

@k8s-github-robot
Copy link

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

@k8s-github-robot
Copy link

Automatic merge from submit-queue (batch tested with PRs 43777, 44121)

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. kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants