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.

Sign up for GitHub

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

Jump to bottom

Add a convenience function for copying a client #2887

Merged
merged 3 commits into from
Jun 20, 2017
Merged

Add a convenience function for copying a client #2887

merged 3 commits into from
Jun 20, 2017

Conversation

sethvargo
Copy link
Contributor

No description provided.

@jefferai
Copy link
Member

Looks fine, one request: it's a bit more Go-ish (although I don't know about HC-ish) to use Clone() instead of Copy()

@jefferai jefferai added this to the 0.7.4 milestone Jun 19, 2017
@sethvargo
Copy link
Contributor Author

@jefferai updated

@jefferai
Copy link
Member

Any reason to ignore the error? I'd just have the Clone function return (*Client, err) the way NewClient does.

@sethvargo
Copy link
Contributor Author

@jefferai a few reasons:

  1. It should never fail. If we already have a client, making a copy of that client should always be successful
  2. It makes it much easier to "chain" client.Clone().Logical()

I wouldn't be opposed to panicing if the clone fails, but it feels a bit weird to return an error here.

@jefferai
Copy link
Member

Copying a client requires making calls (for instance to http2.ConfigureTransport that can error. So either we'd have to swallow that error, or deal with it somehow.

@sethvargo
Copy link
Contributor Author

@jefferai okay updated

@jefferai
Copy link
Member

Thanks!

@jefferai jefferai merged commit a95649a into master Jun 20, 2017
@jefferai jefferai deleted the sethvargo/client_copy branch June 20, 2017 03:08
chrishoffman pushed a commit that referenced this pull request Jun 21, 2017
Merge remote-tracking branch 'oss/master' into db-phy-cockroachdb
3b9b37c 
* oss/master: (161 commits)
  update gitignore
  changelog++
  Exclude /sys/leases/renew from registering with expiration manager (#2891)
  More cleanup
  Clarify/fix some configuration info.
  Add a convenience function for copying a client (#2887)
  Better error messages using ListObjects than using HeadBucket. Might be a bigger request but messages are better than BadRequest, how this changes effect the messages are in the issue (#2892)
  Add ACL info to Consul configuration page
  Return error on bad CORS and add Header specification to API request primitive
  Add Zyborg.Vault PowerShell module to libs list (#2869)
  changelog++
  CouchDB physical backend (#2880)
  Fix root paths test
  Add missing datadog vendored lib
  changelog++
  Fix up CORS.
  Cors headers (#2021)
  Address review feedback
  Fix the test error message
  Added utility on router to fetch mount entry using its ID
  ...
@jefferai jefferai modified the milestones: 0.7.4, 0.8.0 Jul 24, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
0.8.0
Development

Successfully merging this pull request may close these issues.
2 participants
@sethvargo @jefferai