resolve comments

Author: jwtty

Makefile

@@ -323,8 +323,8 @@ manifests: $(CONTROLLER_GEN)
 protos: $(PROTOC_GEN_GO) $(PROTOC_GEN_GO_GRPC) $(PROTOC_GEN_GRPC_GATEWAY) $(PROTOC)
 	PATH=$$PATH:$(TOOLS_BIN_DIR) $(PROTOC) --go_out=. --go_opt=paths=source_relative \
 	          --go-grpc_out=. --go-grpc_opt=paths=source_relative \
-			  --grpc-gateway_out=grpc_api_configuration=pkg/protos/azure/compute/v1/vmsizerecommender_http.yaml,logtostderr=true:. --grpc-gateway_opt=paths=source_relative,generate_unbound_methods=true \
-			  pkg/protos/azure/compute/v1/vmsizerecommender.proto
+			  --grpc-gateway_out=grpc_api_configuration=apis/protos/azure/compute/v1/vmsizerecommender_http.yaml,logtostderr=true:. --grpc-gateway_opt=paths=source_relative,generate_unbound_methods=true \
+			  apis/protos/azure/compute/v1/vmsizerecommender.proto
 
 # Generate code
 generate: $(CONTROLLER_GEN) protos

…azure/compute/v1/vmsizerecommender.pb.go …azure/compute/v1/vmsizerecommender.pb.gopkg/protos/azure/compute/v1/vmsizerecommender.pb.go renamed to apis/protos/azure/compute/v1/vmsizerecommender.pb.go pkg/protos/azure/compute/v1/vmsizerecommender.pb.go renamed to apis/protos/azure/compute/v1/vmsizerecommender.pb.go

@@ -14,7 +14,7 @@ See the License for the specific language governing permissions and
 limitations under the License.
 */
 
-package capacityclient
+package compute
 
 import (
 	"bytes"
@@ -24,56 +24,63 @@ import (
 	"net/http"
 	"strings"
 
+	"github.com/Azure/azure-sdk-for-go/sdk/azcore/runtime"
 	"google.golang.org/protobuf/encoding/protojson"
 
-	computev1 "go.goms.io/fleet/pkg/protos/azure/compute/v1"
+	computev1 "go.goms.io/fleet/apis/protos/azure/compute/v1"
+	"go.goms.io/fleet/pkg/clients/consts"
 )
 
 const (
 	// recommendationsPathTemplate is the URL path template for VM size recommendations API.
 	recommendationsPathTemplate = "/subscriptions/%s/providers/Microsoft.Compute/locations/%s/vmSizeRecommendations/vmAttributeBased/generate"
 )
 
-// CapacityClientFactory is a function type for creating CapacityClient instances.
-type CapacityClientFactory func(endpoint string, httpClient *http.Client) CapacityClient
+// AttributeBasedVMSizeRecommenderClientFactory is a function type for creating AttributeBasedVMSizeRecommenderClient instances.
+type AttributeBasedVMSizeRecommenderClientFactory func(endpoint string, httpClient *http.Client) AttributeBasedVMSizeRecommenderClient
 
-// CapacityClient is an interface for interacting with the Azure Capacity API.
-type CapacityClient interface {
+// AttributeBasedVMSizeRecommenderClient is an interface for interacting with the Azure Attribute-Based VM Size Recommender API.
+type AttributeBasedVMSizeRecommenderClient interface {
 	// GenerateAttributeBasedRecommendations generates VM size recommendations based on attributes.
 	GenerateAttributeBasedRecommendations(ctx context.Context, req *computev1.GenerateAttributeBasedRecommendationsRequest) (*computev1.GenerateAttributeBasedRecommendationsResponse, error)
 }
 
-var _ CapacityClient = &client{}
+var _ AttributeBasedVMSizeRecommenderClient = &attributeBasedVMSizeRecommenderClient{}
 
-// client implements the CapacityClient interface for interacting with Azure Capacity API.
-type client struct {
-	// baseURL is the base URL of the capacity service endpoint.
+// attributeBasedVMSizeRecommenderClient implements the AttributeBasedVMSizeRecommenderClient interface
+// for interacting with Azure Attribute-Based VM Size Recommender API.
+type attributeBasedVMSizeRecommenderClient struct {
+	// baseURL is the base URL of the http(s) requests to the attribute-based VM size recommender service endpoint.
 	baseURL string
 	// httpClient is the HTTP client used for making requests.
 	httpClient *http.Client
 }
 
-// NewClient creates a new capacity client with the given endpoint and HTTP client.
-// If httpClient is nil, http.DefaultClient will be used.
-// If the endpoint does not have a scheme (http:// or https://), like localhost:8080, http:// will be added.
-func NewClient(endpoint string, httpClient *http.Client) CapacityClient {
+// NewAttributeBasedVMSizeRecommenderClient creates a new attribute-based VM size recommender client.
+// The serverAddress is the remote Azure Attribute-Based VM Size Recommender service endpoint.
+// If httpClient is nil, a default client with 60s timeout will be used.
+// If the serverAddress does not have a scheme (http:// or https://), like localhost:8080, http:// will be added.
+func NewAttributeBasedVMSizeRecommenderClient(serverAddress string, httpClient *http.Client) AttributeBasedVMSizeRecommenderClient {
 	if httpClient == nil {
-		httpClient = http.DefaultClient
+		httpClient = &http.Client{Timeout: consts.HTTPTimeoutAzure} // Client with default transport and 60s timeout.
 	}
 	// Add http:// scheme if no scheme is present
-	if !strings.HasPrefix(endpoint, "http://") && !strings.HasPrefix(endpoint, "https://") {
-		endpoint = "http://" + endpoint
+	if !strings.HasPrefix(serverAddress, "http://") && !strings.HasPrefix(serverAddress, "https://") {
+		serverAddress = "http://" + serverAddress
 	}
-	// Ensure endpoint doesn't have trailing slash
-	endpoint = strings.TrimSuffix(endpoint, "/")
-	return &client{
-		baseURL:    endpoint,
+	// Ensure serverAddress doesn't have trailing slash
+	serverAddress = strings.TrimSuffix(serverAddress, "/")
+	return &attributeBasedVMSizeRecommenderClient{
+		baseURL:    serverAddress,
 		httpClient: httpClient,
 	}
 }
 
 // GenerateAttributeBasedRecommendations generates VM size recommendations based on attributes.
-func (c *client) GenerateAttributeBasedRecommendations(ctx context.Context, req *computev1.GenerateAttributeBasedRecommendationsRequest) (*computev1.GenerateAttributeBasedRecommendationsResponse, error) {
+func (c *attributeBasedVMSizeRecommenderClient) GenerateAttributeBasedRecommendations(
+	ctx context.Context,
+	req *computev1.GenerateAttributeBasedRecommendationsRequest,
+) (*computev1.GenerateAttributeBasedRecommendationsResponse, error) {
 	if req == nil {
 		return nil, fmt.Errorf("request cannot be nil")
 	}
@@ -108,8 +115,8 @@ func (c *client) GenerateAttributeBasedRecommendations(ctx context.Context, req
 	}
 
 	// Set headers
-	httpReq.Header.Set("Content-Type", "application/json")
-	httpReq.Header.Set("Accept", "application/json")
+	httpReq.Header.Set(consts.HeaderContentTypeKey, consts.HeaderContentTypeJSON)
+	httpReq.Header.Set(consts.HeaderAcceptKey, consts.HeaderContentTypeJSON)
 
 	// Execute the request
 	resp, err := c.httpClient.Do(httpReq)
@@ -126,7 +133,7 @@ func (c *client) GenerateAttributeBasedRecommendations(ctx context.Context, req
 
 	// Check status code
 	if resp.StatusCode < 200 || resp.StatusCode >= 300 {
-		return nil, fmt.Errorf("request failed with status %d: %s", resp.StatusCode, string(respBody))
+		return nil, fmt.Errorf("request failed with status %d: %w", resp.StatusCode, runtime.NewResponseError(resp))
 	}
 
 	// Unmarshal response using protojson for proper proto3 support

…re/compute/v1/vmsizerecommender.pb.gw.go …re/compute/v1/vmsizerecommender.pb.gw.gopkg/protos/azure/compute/v1/vmsizerecommender.pb.gw.go renamed to apis/protos/azure/compute/v1/vmsizerecommender.pb.gw.go pkg/protos/azure/compute/v1/vmsizerecommender.pb.gw.go renamed to apis/protos/azure/compute/v1/vmsizerecommender.pb.gw.go

@@ -14,7 +14,7 @@ See the License for the specific language governing permissions and
 limitations under the License.
 */
 
-package capacityclient
+package compute
 
 import (
 	"context"
@@ -25,13 +25,16 @@ import (
 	"strings"
 	"testing"
 
+	"github.com/google/go-cmp/cmp"
 	"google.golang.org/protobuf/encoding/protojson"
 	"google.golang.org/protobuf/proto"
 
-	computev1 "go.goms.io/fleet/pkg/protos/azure/compute/v1"
+	computev1 "go.goms.io/fleet/apis/protos/azure/compute/v1"
+	"go.goms.io/fleet/pkg/clients/consts"
 )
 
-func TestNewClient(t *testing.T) {
+func TestNewAttributeBasedVMSizeRecommenderClient(t *testing.T) {
+	defaultClient := &http.Client{Timeout: consts.HTTPTimeoutAzure}
 	tests := []struct {
 		name           string
 		endpoint       string
@@ -51,51 +54,46 @@ func TestNewClient(t *testing.T) {
 			endpoint:       "https://example.com",
 			httpClient:     nil,
 			wantBaseURL:    "https://example.com",
-			wantHTTPClient: http.DefaultClient,
+			wantHTTPClient: defaultClient,
 		},
 		{
 			name:           "removes trailing slash from endpoint",
 			endpoint:       "https://example.com/",
 			httpClient:     nil,
 			wantBaseURL:    "https://example.com",
-			wantHTTPClient: http.DefaultClient,
+			wantHTTPClient: defaultClient,
 		},
 		{
 			name:           "adds http scheme to endpoint without scheme",
 			endpoint:       "localhost:8080",
 			httpClient:     nil,
 			wantBaseURL:    "http://localhost:8080",
-			wantHTTPClient: http.DefaultClient,
+			wantHTTPClient: defaultClient,
 		},
 		{
 			name:           "adds http scheme and removes trailing slash",
 			endpoint:       "example.com:8080/",
 			httpClient:     nil,
 			wantBaseURL:    "http://example.com:8080",
-			wantHTTPClient: http.DefaultClient,
+			wantHTTPClient: defaultClient,
 		},
 		{
 			name:           "preserves existing http scheme",
 			endpoint:       "http://localhost:8080",
 			httpClient:     nil,
 			wantBaseURL:    "http://localhost:8080",
-			wantHTTPClient: http.DefaultClient,
+			wantHTTPClient: defaultClient,
 		},
 	}
 
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			got := NewClient(tt.endpoint, tt.httpClient).(*client)
+			got := NewAttributeBasedVMSizeRecommenderClient(tt.endpoint, tt.httpClient).(*attributeBasedVMSizeRecommenderClient)
 			if got.baseURL != tt.wantBaseURL {
 				t.Errorf("NewClient() baseURL = %v, want %v", got.baseURL, tt.wantBaseURL)
 			}
-			// For custom HTTP client, just verify it's not nil
-			if tt.httpClient != nil && got.httpClient == nil {
-				t.Errorf("NewClient() httpClient is nil, want non-nil")
-			}
-			// For nil HTTP client, verify it uses default client
-			if tt.httpClient == nil && got.httpClient != http.DefaultClient {
-				t.Errorf("NewClient() httpClient = %v, want http.DefaultClient", got.httpClient)
+			if !cmp.Equal(got.httpClient, tt.wantHTTPClient) {
+				t.Errorf("NewClient() httpClient = %v, want %v", got.httpClient, tt.wantHTTPClient)
 			}
 		})
 	}
@@ -240,22 +238,22 @@ func TestClient_GenerateAttributeBasedRecommendations(t *testing.T) {
 			server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
 				// Verify request method
 				if r.Method != http.MethodPost {
-					t.Errorf("expected POST request, got %s", r.Method)
+					t.Errorf("got %s, want POST request", r.Method)
 				}
 
 				// Verify headers
 				if r.Header.Get("Content-Type") != "application/json" {
-					t.Errorf("expected Content-Type: application/json, got %s", r.Header.Get("Content-Type"))
+					t.Errorf("got %s, want Content-Type: application/json", r.Header.Get("Content-Type"))
 				}
 				if r.Header.Get("Accept") != "application/json" {
-					t.Errorf("expected Accept: application/json, got %s", r.Header.Get("Accept"))
+					t.Errorf("got %s, want Accept: application/json", r.Header.Get("Accept"))
 				}
 
 				// Verify URL path if request is not nil
 				if tt.request != nil && tt.request.SubscriptionId != "" && tt.request.Location != "" {
-					expectedPath := fmt.Sprintf(recommendationsPathTemplate, tt.request.SubscriptionId, tt.request.Location)
-					if r.URL.Path != expectedPath {
-						t.Errorf("expected path %s, got %s", expectedPath, r.URL.Path)
+					wantPath := fmt.Sprintf(recommendationsPathTemplate, tt.request.SubscriptionId, tt.request.Location)
+					if r.URL.Path != wantPath {
+						t.Errorf("got %s, want path %s", r.URL.Path, wantPath)
 					}
 
 					// Verify request body using protojson for proper proto3 oneof support
@@ -284,7 +282,7 @@ func TestClient_GenerateAttributeBasedRecommendations(t *testing.T) {
 			defer server.Close()
 
 			// Create client
-			client := NewClient(server.URL, nil)
+			client := NewAttributeBasedVMSizeRecommenderClient(server.URL, nil)
 
 			// Execute request
 			got, err := client.GenerateAttributeBasedRecommendations(context.Background(), tt.request)
@@ -307,31 +305,3 @@ func TestClient_GenerateAttributeBasedRecommendations(t *testing.T) {
 		})
 	}
 }
-
-func TestClient_GenerateAttributeBasedRecommendations_ContextCancellation(t *testing.T) {
-	// Create a server that delays response
-	server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
-		// This handler will never respond in time
-		<-r.Context().Done()
-	}))
-	defer server.Close()
-
-	client := NewClient(server.URL, nil)
-
-	// Create a context that is already cancelled
-	ctx, cancel := context.WithCancel(context.Background())
-	cancel()
-
-	req := &computev1.GenerateAttributeBasedRecommendationsRequest{
-		SubscriptionId: "sub-123",
-		Location:       "eastus",
-		PriorityProfile: &computev1.GenerateAttributeBasedRecommendationsRequest_RegularPriorityProfile{
-			RegularPriorityProfile: &computev1.RegularPriorityProfile{},
-		},
-	}
-
-	_, err := client.GenerateAttributeBasedRecommendations(ctx, req)
-	if err == nil {
-		t.Error("GenerateAttributeBasedRecommendations() expected error for cancelled context, got nil")
-	}
-}

…azure/compute/v1/vmsizerecommender.proto …azure/compute/v1/vmsizerecommender.protopkg/protos/azure/compute/v1/vmsizerecommender.proto renamed to apis/protos/azure/compute/v1/vmsizerecommender.proto pkg/protos/azure/compute/v1/vmsizerecommender.proto renamed to apis/protos/azure/compute/v1/vmsizerecommender.proto

@@ -0,0 +1,34 @@
+/*
+Copyright 2025 The KubeFleet Authors.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package consts
+
+import "time"
+
+// Common HTTP constants.
+const (
+	// HTTPTimeoutAzure is the timeout for HTTP requests to Azure services.
+	// Setting to 60 seconds, following ARM client request timeout conventions:
+	// https://github.com/Azure/azure-resource-manager-rpc/blob/master/v1.0/common-api-details.md#client-request-timeout.
+	HTTPTimeoutAzure = 60 * time.Second
+
+	// HeaderContentTypeKey is the HTTP header key for Content-Type.
+	HeaderContentTypeKey = "Content-Type"
+	// HeaderAcceptKey is the HTTP header key for Accept.
+	HeaderAcceptKey = "Accept"
+	// HeaderContentTypeJSON is the Content-Type header value for JSON payloads.
+	HeaderContentTypeJSON = "application/json"
+)