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

etcdserver: improve Lease http path naming for gRPC gateway #9450

Merged
merged 1 commit into from
Mar 26, 2018

Conversation

hexfusion
Copy link
Contributor

This PR looks to normalize some of the lease api paths for the gRPC gateway. Using additional_bindings supported by grpc-gateway we can accept both old and new paths. Current plan is to support in 3.4 and deprecate all additional_bindings in 3.5. I will work on notifying existing clients of the change as well.

fixes #9430

/cc @akashbaid

@hexfusion
Copy link
Contributor Author

I have no excuse to not add curl leases tests here ...

@codecov-io
Copy link

codecov-io commented Mar 17, 2018

Codecov Report

❗ No coverage uploaded for pull request base (master@31de834). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #9450   +/-   ##
=========================================
  Coverage          ?   72.23%           
=========================================
  Files             ?      364           
  Lines             ?    30887           
  Branches          ?        0           
=========================================
  Hits              ?    22310           
  Misses            ?     6940           
  Partials          ?     1637

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 31de834...cd92d4a. Read the comment docs.

@@ -615,17 +615,7 @@
"Lease"
],
"summary": "LeaseLeases lists all existing leases.",
"operationId": "LeaseLeases",
"parameters": [

Choose a reason for hiding this comment

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

The parameters blocks such as this getting deleted in the auto-generated file doesn't seem right. Maybe a problem in the way "additional-bindings" work?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah something is amiss, it works but not consistently. Possible upstream bug, but not sure.

@gyuho
Copy link
Contributor

gyuho commented Mar 20, 2018

@hexfusion Can we also update changelogs?

@hexfusion
Copy link
Contributor Author

@gyuho sure would you mind reviewing #9460 we need to address semaphore timeouts. Also I have experienced this same code above, passing and then failing Lease tests so more review is required before merge. #9460 will help this effort, thanks!

@hexfusion
Copy link
Contributor Author

@akashbaid, I find the below to be stable and produce proper Swagger. WIll update PR and tests once #9460 gets merged.

  // LeaseLeases lists all existing leases.                                              
  rpc LeaseLeases(LeaseLeasesRequest) returns (LeaseLeasesResponse) {                    
      option (google.api.http) = {                                                       
        post: "/v3/lease/leases"                                                         
        body: "*"                                                                        
        additional_bindings {                                                            
            post: "/v3/kv/lease/leases"                                                  
            body: "*"                                                                    
        }                                                                                
    };                                                                                   
  }

@skydoctor
Copy link

That's great! Thanks for looking into it.

@hexfusion
Copy link
Contributor Author

@gyuho updated release notes and tweaked lease tests for coverage. Should be good on greens.

Copy link
Contributor

@gyuho gyuho left a comment

Choose a reason for hiding this comment

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

lgtm. thanks @hexfusion !

@gyuho gyuho merged commit a994fde into etcd-io:master Mar 26, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Incorrect URIs for some lease related functions in JSON API
4 participants