Add GroupVersion tags to OpenAPI spec and remove all specs except main one #35388

mbohlool · 2016-10-23T06:55:55Z

Tags are used as a grouping mechanism in OpenAPI. We generated one spec per GroupVersion before for this grouping but by adding those tags in this PR, those files have no use. We can always add them back if there were a use-case for them.

Release note:

Deprecate OpenAPI spec for GroupVersion endpoints in favor of single spec /swagger.json

Reference: #13414

This change is

mbohlool · 2016-10-23T07:01:21Z

cc @bgrant0607 @thockin @lavalamp @kubernetes/sig-api-machinery

Please look at this change as it is a design change.

mbohlool · 2016-10-23T07:03:48Z

hack/update-federation-openapi-spec.sh

@@ -60,18 +60,15 @@ kube::log::status "Starting federation-apiserver"
  --insecure-port="${API_PORT}" \
  --etcd-servers="http://${ETCD_HOST}:${ETCD_PORT}" \
  --advertise-address="10.10.10.10" \
+  --cert-dir="${TMP_DIR}/certs" \


@nikhiljindal this is the reason federation server did not start on my machine.

k8s-ci-robot · 2016-10-24T06:50:30Z

Jenkins unit/integration failed for commit 9d3a79346020a6174475502613152e65f44eba3a. Full PR test history.

The magic incantation to run this job again is @k8s-bot unit test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

caesarxuchao

+2,012 −48,660 is cool! Just curious, I thought we just merged the swagger into one file, why would the PR remove so many lines? Is it because there were many duplicate swagger definition before?

Also out of curiosity, when generating code, will the swagger paths with the same tags be generated as one file? What generating tool are we using?

caesarxuchao · 2016-10-25T05:19:32Z

pkg/apiserver/openapi/openapi.go

@@ -72,10 +73,15 @@ func GetOperationID(servePath string, r *restful.Route) (string, error) {
 			prefix = prefix + ToValidOperationID(strings.TrimSuffix(parts[1], ".k8s.io"), prefix != "")
 			if len(parts) > 2 {
 				prefix = prefix + ToValidOperationID(parts[2], prefix != "")
+				tags = append(tags, fmt.Sprintf("%s_%s", ToValidOperationID(parts[2], false), ToValidOperationID(strings.TrimSuffix(parts[1], ".k8s.io"), false)))


Why version_group rather than group_version?

That was Daniel's suggestion for spec filenames and I just followed naming pattern here. @nikhiljindal knows?

I dont see a reason for version_group. I think group_version or group/version or group.version should be fine which is what we use at other places.

Please don't use . as the separator. Group might have . in the name. (Though I remember you removed .k8s.io suffix)

Yeah, I've used group_version.

smarterclayton · 2016-10-25T18:24:31Z

I think we should always do GroupVersion because Group dominates Version.

On Tue, Oct 25, 2016 at 6:01 AM, mbohlool notifications@github.com wrote:

@mbohlool commented on this pull request.

In pkg/apiserver/openapi/openapi.go
#35388:
@@ -72,10 +73,15 @@ func GetOperationID(servePath string, r *restful.Route) (string, error) {
prefix = prefix + ToValidOperationID(strings.TrimSuffix(parts[1], ".k8s.io"), prefix != "")
if len(parts) > 2 {
prefix = prefix + ToValidOperationID(parts[2], prefix != "")
        tags = append(tags, fmt.Sprintf("%s_%s", ToValidOperationID(parts[2], false), ToValidOperationID(strings.TrimSuffix(parts[1], ".k8s.io"), false)))
That was Daniel's suggestion for spec filenames and I just followed naming
pattern here. @nikhiljindal https://github.com/nikhiljindal knows?

—
You are receiving this because you are on a team that was mentioned.
Reply to this email directly, view it on GitHub
#35388, or mute the thread
https://github.com/notifications/unsubscribe-auth/ABG_py3zGS_VPQNTyiiOUCkgArHyqDYqks5q3dNqgaJpZM4KeEM2
.

k8s-ci-robot · 2016-10-25T20:51:37Z

Jenkins verification failed for commit a0435a6a965ffc7ebf1aa0550147ee6ef902364a. Full PR test history.

The magic incantation to run this job again is @k8s-bot verify test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

nikhiljindal · 2016-10-25T21:12:43Z

LGTM once the group_version or version_group comment is resolved. Thx!

mbohlool · 2016-10-25T21:27:03Z

@caesarxuchao We were generating swagger.json for all endpoints as well as a root one. This change will only keep root one and remove all those specs for endpoints, that why we have a lot of removed line.

Generators (at least the one I've tried) generate one file (or class) for each tag.

- Remove all end-point specs as they are not useful in light of GroupVersion tags in main spec

caesarxuchao · 2016-10-25T21:37:12Z

LGTM. Thanks.

k8s-github-robot · 2016-10-26T03:48:22Z

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

k8s-github-robot · 2016-10-26T04:25:37Z

Automatic merge from submit-queue

liggitt · 2016-10-27T23:34:35Z

Does this mean we no longer serve openapi specs per group? I thought we were trying to allow decentralizing discovery and schema info

mbohlool · 2016-10-28T18:27:28Z

It is easy to convert the existing spec to any of the group spec (group them by tag). If we see a usecase for it, we can always bring it back (or write a simple tool to do the conversion) but we think it is easier to maintain this way. It is also possible (with little effort) to generate a client and pick the parts that you need (each tag will result in an API file. Before we had N spec and N API files, right now we have one spec and still N API files. If you only need core_v1, you can pick ApiCoreVi1.py (for python for example) the remove the rest.

liggitt · 2016-10-28T18:46:06Z

I'm more concerned about compatibility implications for clients that were fetching per group/version specs for validation (like kubectl). I couldn't tell if this was changing what was served by the API server.

mbohlool · 2016-10-28T20:43:16Z

Oh, that is still swagger 1.2 that clients such as kubectl using and that is not changed. We are going to keep them for a while until we upgrade all clients to OpenAPI.
This change is only about OpenAPI group specific specs, and they were added in the 1.5 development timeline, so they've never been release. Actually that was one of the reasons we removed them. We do not use them, we do not see any usecase for them, and it would be a compatibility issue to remove them after 1.5 release.

Automatic merge from submit-queue Update verify-openapi-spec script to check for extra generated spec hack/verify-openapi-spec.sh only check for existing spec changes. If for some reason (here most probably I forgot to delete a file in api/openapi-spec folder in #35388 after a rebase) there is an old spec exists in the spec folder, it won't panic but it should. This resulted in an unused out of date v1.spec file in the api/openapi-spec folder that this PR also removes.

mbohlool assigned caesarxuchao and nikhiljindal Oct 23, 2016

googlebot added the cla: yes label Oct 23, 2016

k8s-github-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. release-note-label-needed labels Oct 23, 2016

mbohlool commented Oct 23, 2016

View reviewed changes

mbohlool force-pushed the co2 branch from 27fb1f4 to 9d3a793 Compare October 24, 2016 06:35

mbohlool force-pushed the co2 branch 3 times, most recently from ed011bd to 0b0a606 Compare October 24, 2016 09:53

caesarxuchao reviewed Oct 25, 2016

View reviewed changes

k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 25, 2016

mbohlool force-pushed the co2 branch from 0b0a606 to a0435a6 Compare October 25, 2016 20:26

mbohlool force-pushed the co2 branch from a0435a6 to e51c0e5 Compare October 25, 2016 21:00

k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 25, 2016

mbohlool added 2 commits October 25, 2016 14:27

- Add GroupVersion as tags to OpenAPI spec

75451b4

- Remove all end-point specs as they are not useful in light of GroupVersion tags in main spec

Update OpenAPI specs

cf181f9

mbohlool force-pushed the co2 branch from e51c0e5 to cf181f9 Compare October 25, 2016 21:28

caesarxuchao added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 25, 2016

k8s-github-robot added the do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. label Oct 25, 2016

mbohlool added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-label-needed do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. labels Oct 25, 2016

mbohlool added this to the v1.5 milestone Oct 26, 2016

k8s-github-robot merged commit fc7f64a into kubernetes:master Oct 26, 2016

This was referenced Oct 29, 2016

Update verify-openapi-spec script to check for extra generated spec #35841

Merged

Remove unnecessary prefixes from OperationIDs in OpenAPI spec #36002

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add GroupVersion tags to OpenAPI spec and remove all specs except main one #35388

Add GroupVersion tags to OpenAPI spec and remove all specs except main one #35388

mbohlool commented Oct 23, 2016 •

edited

Loading

mbohlool commented Oct 23, 2016 •

edited

Loading

mbohlool Oct 23, 2016

nikhiljindal Oct 25, 2016

k8s-ci-robot commented Oct 24, 2016

caesarxuchao left a comment

caesarxuchao Oct 25, 2016

mbohlool Oct 25, 2016

nikhiljindal Oct 25, 2016 •

edited

Loading

caesarxuchao Oct 25, 2016

mbohlool Oct 25, 2016

smarterclayton commented Oct 25, 2016

@mbohlool commented on this pull request.

k8s-ci-robot commented Oct 25, 2016

nikhiljindal commented Oct 25, 2016

mbohlool commented Oct 25, 2016 •

edited

Loading

caesarxuchao commented Oct 25, 2016

k8s-github-robot commented Oct 26, 2016

k8s-github-robot commented Oct 26, 2016

liggitt commented Oct 27, 2016

mbohlool commented Oct 28, 2016

liggitt commented Oct 28, 2016

mbohlool commented Oct 28, 2016

Add GroupVersion tags to OpenAPI spec and remove all specs except main one #35388

Add GroupVersion tags to OpenAPI spec and remove all specs except main one #35388

Conversation

mbohlool commented Oct 23, 2016 • edited Loading

mbohlool commented Oct 23, 2016 • edited Loading

mbohlool Oct 23, 2016

Choose a reason for hiding this comment

nikhiljindal Oct 25, 2016

Choose a reason for hiding this comment

k8s-ci-robot commented Oct 24, 2016

caesarxuchao left a comment

Choose a reason for hiding this comment

caesarxuchao Oct 25, 2016

Choose a reason for hiding this comment

mbohlool Oct 25, 2016

Choose a reason for hiding this comment

nikhiljindal Oct 25, 2016 • edited Loading

Choose a reason for hiding this comment

caesarxuchao Oct 25, 2016

Choose a reason for hiding this comment

mbohlool Oct 25, 2016

Choose a reason for hiding this comment

smarterclayton commented Oct 25, 2016

@mbohlool commented on this pull request.

k8s-ci-robot commented Oct 25, 2016

nikhiljindal commented Oct 25, 2016

mbohlool commented Oct 25, 2016 • edited Loading

caesarxuchao commented Oct 25, 2016

k8s-github-robot commented Oct 26, 2016

k8s-github-robot commented Oct 26, 2016

liggitt commented Oct 27, 2016

mbohlool commented Oct 28, 2016

liggitt commented Oct 28, 2016

mbohlool commented Oct 28, 2016

mbohlool commented Oct 23, 2016 •

edited

Loading

mbohlool commented Oct 23, 2016 •

edited

Loading

nikhiljindal Oct 25, 2016 •

edited

Loading

mbohlool commented Oct 25, 2016 •

edited

Loading