hetznercloud / hcloud-go

A Go library for the Hetzner Cloud API
https://pkg.go.dev/github.com/hetznercloud/hcloud-go/v2/hcloud
MIT License
384 stars 45 forks source link

Inconsistency in Load Balancer API #520

Closed phm07 closed 1 month ago

phm07 commented 2 months ago

Currently, there are these methods to manage a Load Balancer's targets:

There are several problems/inconsistencies with these methods:

I propose two new methods AddTarget and RemoveTarget, which would replicate the API endpoint exactly by allowing to add/remove all three kinds of targets with only one method. This would be more flexible, intuitive and idiomatic. The old Add/Remove targets could be deprecated, so this change wouldn't be breaking.

apricote commented 2 months ago

specialized vs general methods

I think having different structs as the opts is actually easier for the user.

All three options take different parameters, so cramming them into a single Opts struct requires additional documentation and understanding.

type AddTargetOpts {
  // Only one of these may be set. How do we handle if the user sets multiple?
  Server   *Server
  Selector *string
  IP       *net.IP

  // Only valid if Server or Selector was set. What do we do if IP was set? Ignore or error?
  UsePrivateIP *bool
}

Though a general method would match the API more closely.

Opts

Using Opts is always nicer, as we can amend the opts if new fields are added to the API.

Historically it looks like hcloud-go only used Opts when the endpoint actually had more than one parameter, and otherwise put the one parameter directly in the function arguments.

Happy to change this in v3.

phm07 commented 2 months ago

I think having different structs as the opts is actually easier for the user.

I see your point. Although you could argue that the current hcloud-go pattern causes more code duplication, not only on our end, but on the user end too. But that's besides the point anyway. The API already determined the pattern by only using a single endpoint. I don't think we should deviate from the API and add an extra abstraction layer just because we think that one API is prettier or easier to use than the other one. I much prefer a consistent experience between API and hcloud-go.

All three options take different parameters, so cramming them into a single Opts struct requires additional documentation and understanding.

The API docs already document this, since it is exactly what the API does. If anything, we have to document it differently when we deviate from the API.

// Only one of these may be set. How do we handle if the user sets multiple?

The API already handles it using the type property (which you forgot to include in the struct). We wouldn't have to add any additional logic.

// Only valid if Server or Selector was set. What do we do if IP was set? Ignore or error?

Again, we don't have to add any additional logic. We can just forward it to the API and will receive errors (or not) accordingly.

Using Opts is always nicer, as we can amend the opts if new fields are added to the API.

I agree. More consistency also means that code generation is easier (if we want to do that in the future)

phm07 commented 1 month ago

We came to the conclusion that we want to provide a user-friendly way to represent oneOf schemas. (Same for managed/uploaded certificates). Since this is a bigger topic and only makes sense to discuss for a v3 I will close this issue.