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

Improve the output of the swagger API for watch events #6232

Merged

Conversation

smarterclayton
Copy link
Contributor

Stopgap to improve this prior to converting watch resources to
versioned objects.

Spawned from #6182

@googlebot
Copy link

Thanks for your pull request.

It looks like this may be your first contribution to a Google open source project, in which case you'll need to sign a Contributor License Agreement (CLA) at https://cla.developers.google.com/.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check the information on your CLA or see this help article on setting the email on your git commits.

Once you've done that, please reply here to let us know. If you signed the CLA as a corporation, please let us know the company's name.

@bgrant0607
Copy link
Member

LGTM. Thanks. @thockin can rebase on top of this and regenerate the swagger.

@bgrant0607 bgrant0607 added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 31, 2015
@brendandburns
Copy link
Contributor

Tests are failing:

# github.com/GoogleCloudPlatform/kubernetes/pkg/watch/json
_output/local/go/src/github.com/GoogleCloudPlatform/kubernetes/pkg/watch/json/decoder_test.go:45: undefined: watchEvent
FAIL    github.com/GoogleCloudPlatform/kubernetes/pkg/watch/json [build failed]

@bgrant0607 bgrant0607 removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 31, 2015
@bgrant0607
Copy link
Member

Merged multi-port services. Please run hack/update-swagger-spec.sh.

@smarterclayton smarterclayton force-pushed the minor_fixes_to_watch_swagger branch 2 times, most recently from 18726d9 to 54162c5 Compare March 31, 2015 21:05
@smarterclayton
Copy link
Contributor Author

Rebased, test fixed, and docs updated.

----- Original Message -----

Merged multi-port services. Please run hack/update-swagger-spec.sh.


Reply to this email directly or view it on GitHub:
#6232 (comment)

"v1beta3.ServiceSpec": {
"id": "v1beta3.ServiceSpec",
"required": [
"ports",
"port",
Copy link
Member

Choose a reason for hiding this comment

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

Something's not right here. This is reverting multi-port services, among other things.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Bad merge, let me delete and regen.

----- Original Message -----

"v1beta3.ServiceSpec": {
 "id": "v1beta3.ServiceSpec",
 "required": [
  • "ports",
    
  • "port",
    

Something's not right here. This is reverting multi-port services, among
other things.


Reply to this email directly or view it on GitHub:
https://github.com/GoogleCloudPlatform/kubernetes/pull/6232/files#r27528460

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, sorry.

----- Original Message -----

Bad merge, let me delete and regen.

----- Original Message -----

"v1beta3.ServiceSpec": {
 "id": "v1beta3.ServiceSpec",
 "required": [
  • "ports",
    
  • "port",
    

Something's not right here. This is reverting multi-port services, among
other things.


Reply to this email directly or view it on GitHub:
https://github.com/GoogleCloudPlatform/kubernetes/pull/6232/files#r27528460

@bgrant0607
Copy link
Member

Weird. I'm not seeing the comments I received via email, nor the latest push.

@bgrant0607
Copy link
Member

@smarterclayton Maybe try to make clean, rebuild, and regenerate the swagger?

@nikhiljindal Any guess about what's going on with the swagger generation? A stale go-restful dependency?

@nikhiljindal
Copy link
Contributor

@smarterclayton : Did you rebuild the binary before running update-swagger-spec?
That script uses the existing binary rather then re-building it.

@bgrant0607
Copy link
Member

@nikhiljindal He picked up some recent API changes, so I'd guess he did build it.

@nikhiljindal
Copy link
Contributor

The type changes and addition of a path parameter in swagger spec are coming from #6143 and #6117.

@bgrant0607
Copy link
Member

Ok, sorry. Swagger has been updated from earlier changes. Please rebase and re-update swagger again. Feel free to self-merge.

@bgrant0607 bgrant0607 added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 1, 2015
@bgrant0607
Copy link
Member

@smarterclayton Could we try to get this in before something breaks it again? :-) It might be easier if you didn't update the swagger in this PR.

Stopgap to improve this prior to converting watch resources to
versioned objects.
@smarterclayton
Copy link
Contributor Author

Rebased, split swagger out.

@bgrant0607
Copy link
Member

LGTM. Thanks.

When looking at this I noticed that the /log content types are wrong.

The shippable failure is a known kubelet test problem.

bgrant0607 added a commit that referenced this pull request Apr 7, 2015
…gger

Improve the output of the swagger API for watch events
@bgrant0607 bgrant0607 merged commit 6e93d37 into kubernetes:master Apr 7, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lgtm "Looks good to me", indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants