feat: add attributed based recommender client sdk #1213
Conversation
| serverAddress = "http://" + serverAddress | ||
| } | ||
| // Ensure serverAddress doesn't have trailing slash | ||
| serverAddress = strings.TrimSuffix(serverAddress, "/") |
There was a problem hiding this comment.
is this not always localhost?
There was a problem hiding this comment.
Should be http://localhost:<some port>, I prefer the port to be specified from argument instead of hardcoded.
jwtty
force-pushed
the
recomm-client
branch
2 times, most recently
from
8120e8a to
fac9f1e
Compare
|
|
||
| // attributeBasedVMSizeRecommenderClient implements the AttributeBasedVMSizeRecommenderClient interface | ||
| // for interacting with Azure Attribute-Based VM Size Recommender API. | ||
| type attributeBasedVMSizeRecommenderClient struct { |
There was a problem hiding this comment.
need to make it public as a producer and NewAttributeBasedVMSizeRecommenderClient should return the concrete type.
| ) | ||
|
|
||
| // AttributeBasedVMSizeRecommenderClient is an interface for interacting with the Azure Attribute-Based VM Size Recommender API. | ||
| type AttributeBasedVMSizeRecommenderClient interface { |
There was a problem hiding this comment.
The interface should be defined in the consumer side.
And it may be better to define the interface where we use it and we can have a fake implementation there.
There was a problem hiding this comment.
I see a bunch of interfaces defined with the client library itself. I think the point is to provide a self-contained library so users can directly use it: https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/client-go/dynamic/dynamicinformer/interface.go#L26
| httpClient *http.Client | ||
| } | ||
|
|
||
| // NewAttributeBasedVMSizeRecommenderClient creates a new attribute-based VM size recommender client. |
There was a problem hiding this comment.
The current implementation is anti-pattern,
// Bad:
package producer
type Thinger interface { Thing() bool }
type defaultThinger struct{ ... }
func (t defaultThinger) Thing() bool { ... }
func NewThinger() Thinger { return defaultThinger{ ... } }
There was a problem hiding this comment.
More details could be found in https://google.github.io/styleguide/go/decisions#interfaces
There was a problem hiding this comment.
I see a lot of similar style: an example: https://github.com/argoproj/argo-cd/blob/d8c72c2c8ecf6a16f05fcac405de8c0728f0359f/applicationset/services/pull_request/azure_devops.go#L42C1-L63C2
There was a problem hiding this comment.
zhiying-lin
left a comment
zhiying-lin
left a comment
There was a problem hiding this comment.
Approved to unblock the project progress.
The interface style is also mentioned in https://go.dev/wiki/CodeReviewComments#interfaces.
I'll leave the decision to you, as it's more like a style issue.
4 participants
Description of your changes
This PR adds the proto and sdk for querying the AttributedBaseRecommendations API.
Tested in standalone env with custom client: fee0b52
And configured with env
CAPACITY_ENDPOINTas:and
Both worked:
Fixes #
I have:
make reviewableto ensure this PR is ready for review.How has this code been tested
Special notes for your reviewer