Skip to content

Commit ed86242

Browse files
yogesh-chauhanHarness
yogesh-chauhan
authored and
Harness
committed
feat: [ML-1459]: Add License Factory to create license client (#238)
* 23b251 refactor: simplify license client factory by removing unused account-specific methods * 864fb1 code format * 24167e feat: [ML-1459]: Add License Factory to create license client
1 parent 18e4d0c commit ed86242

File tree

3 files changed

+365
-10
lines changed

3 files changed

+365
-10
lines changed

‎pkg/harness/tools.go‎

Lines changed: 4 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -9,9 +9,9 @@ import (
99
"time"
1010

1111
"github.com/harness/harness-mcp/client"
12-
"github.com/harness/harness-mcp/client/license"
1312
"github.com/harness/harness-mcp/cmd/harness-mcp-server/config"
1413
"github.com/harness/harness-mcp/pkg/harness/tools"
14+
licenseFactory "github.com/harness/harness-mcp/pkg/license"
1515
"github.com/harness/harness-mcp/pkg/modules"
1616
"github.com/harness/harness-mcp/pkg/modules/utils"
1717
"github.com/harness/harness-mcp/pkg/toolsets"
@@ -77,15 +77,9 @@ func initLicenseValidation(ctx context.Context, config *config.Config) (*License
7777
IsValid: false,
7878
}
7979

80-
// Use the NGManager service for license validation
81-
licenseClient, err := license.CreateCustomLicenseClientWithContext(
82-
ctx,
83-
config,
84-
config.NgManagerBaseURL,
85-
config.BaseURL,
86-
"ng/api",
87-
config.NgManagerSecret,
88-
)
80+
// Use the shared license client factory for consistent license client creation
81+
licenseFactory := licenseFactory.NewClientFactory(config, slog.Default())
82+
licenseClient, err := licenseFactory.CreateLicenseClient(ctx)
8983
if err != nil {
9084
return licenseInfo, fmt.Errorf("failed to create license client, error: %w", err)
9185
}

‎pkg/license/client_factory.go‎

Lines changed: 78 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,78 @@
1+
package license
2+
3+
import (
4+
"context"
5+
"log/slog"
6+
"sync"
7+
8+
"github.com/harness/harness-mcp/client/license"
9+
"github.com/harness/harness-mcp/cmd/harness-mcp-server/config"
10+
)
11+
12+
// ClientFactory provides a centralized way to create license clients
13+
// This eliminates DRY violations by providing a single source of truth for license client creation
14+
type ClientFactory struct {
15+
config *config.Config
16+
logger *slog.Logger
17+
18+
// Single instance of license client
19+
client *license.CustomLicensesApiService
20+
clientMu sync.RWMutex
21+
initialized bool
22+
}
23+
24+
// NewClientFactory creates a new license client factory
25+
func NewClientFactory(config *config.Config, logger *slog.Logger) *ClientFactory {
26+
return &ClientFactory{
27+
config: config,
28+
logger: logger,
29+
initialized: false,
30+
}
31+
}
32+
33+
// CreateLicenseClient creates a license client using the standard configuration
34+
// This method encapsulates the common license client creation logic used across the application
35+
// It reuses a single client instance if already created
36+
func (f *ClientFactory) CreateLicenseClient(ctx context.Context) (*license.CustomLicensesApiService, error) {
37+
// Check if we already have a client instance
38+
f.clientMu.RLock()
39+
if f.initialized && f.client != nil {
40+
client := f.client
41+
f.clientMu.RUnlock()
42+
return client, nil
43+
}
44+
f.clientMu.RUnlock()
45+
46+
// Need to create a new client, acquire write lock
47+
f.clientMu.Lock()
48+
defer f.clientMu.Unlock()
49+
50+
// Double-check that another goroutine hasn't created the client while we were waiting
51+
if f.initialized && f.client != nil {
52+
f.logger.Debug("Using existing license client (after lock)")
53+
return f.client, nil
54+
}
55+
56+
f.logger.Debug("Creating new license client")
57+
58+
// Use the NGManager service for license validation - same pattern as tools.go
59+
licenseClient, err := license.CreateCustomLicenseClientWithContext(
60+
ctx,
61+
f.config,
62+
f.config.NgManagerBaseURL,
63+
f.config.BaseURL,
64+
"ng/api",
65+
f.config.NgManagerSecret,
66+
)
67+
if err != nil {
68+
f.logger.Error("Failed to create license client", "error", err)
69+
return nil, err
70+
}
71+
72+
// Store the client instance
73+
f.client = licenseClient
74+
f.initialized = true
75+
76+
f.logger.Debug("Successfully created license client")
77+
return licenseClient, nil
78+
}

‎pkg/license/client_factory_test.go‎

Lines changed: 283 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,283 @@
1+
package license
2+
3+
import (
4+
"context"
5+
"log/slog"
6+
"os"
7+
"testing"
8+
9+
"github.com/harness/harness-mcp/cmd/harness-mcp-server/config"
10+
"github.com/stretchr/testify/assert"
11+
"github.com/stretchr/testify/require"
12+
)
13+
14+
func TestNewClientFactory(t *testing.T) {
15+
tests := []struct {
16+
name string
17+
config *config.Config
18+
logger *slog.Logger
19+
want *ClientFactory
20+
}{
21+
{
22+
name: "valid config and logger",
23+
config: &config.Config{
24+
AccountID: "test-account",
25+
NgManagerBaseURL: "https://app.harness.io",
26+
BaseURL: "https://app.harness.io",
27+
NgManagerSecret: "test-secret",
28+
},
29+
logger: slog.New(slog.NewTextHandler(os.Stdout, nil)),
30+
want: &ClientFactory{
31+
config: &config.Config{
32+
AccountID: "test-account",
33+
NgManagerBaseURL: "https://app.harness.io",
34+
BaseURL: "https://app.harness.io",
35+
NgManagerSecret: "test-secret",
36+
},
37+
logger: slog.New(slog.NewTextHandler(os.Stdout, nil)),
38+
},
39+
},
40+
{
41+
name: "nil config",
42+
config: nil,
43+
logger: slog.New(slog.NewTextHandler(os.Stdout, nil)),
44+
want: &ClientFactory{
45+
config: nil,
46+
logger: slog.New(slog.NewTextHandler(os.Stdout, nil)),
47+
},
48+
},
49+
{
50+
name: "nil logger",
51+
config: &config.Config{
52+
AccountID: "test-account",
53+
},
54+
logger: nil,
55+
want: &ClientFactory{
56+
config: &config.Config{
57+
AccountID: "test-account",
58+
},
59+
logger: nil,
60+
},
61+
},
62+
}
63+
64+
for _, tt := range tests {
65+
t.Run(tt.name, func(t *testing.T) {
66+
got := NewClientFactory(tt.config, tt.logger)
67+
68+
require.NotNil(t, got)
69+
assert.Equal(t, tt.config, got.config)
70+
71+
// For logger comparison, we check if both are nil or both are not nil
72+
// since slog.Logger doesn't have a direct equality comparison
73+
if tt.logger == nil {
74+
assert.Nil(t, got.logger)
75+
} else {
76+
assert.NotNil(t, got.logger)
77+
}
78+
})
79+
}
80+
}
81+
82+
func TestClientFactory_CreateLicenseClient(t *testing.T) {
83+
tests := []struct {
84+
name string
85+
config *config.Config
86+
expectError bool
87+
errorContains string
88+
}{
89+
{
90+
name: "valid config",
91+
config: &config.Config{
92+
AccountID: "test-account",
93+
NgManagerBaseURL: "https://app.harness.io",
94+
BaseURL: "https://app.harness.io",
95+
NgManagerSecret: "test-secret",
96+
},
97+
expectError: false,
98+
},
99+
{
100+
name: "empty NgManagerBaseURL",
101+
config: &config.Config{
102+
AccountID: "test-account",
103+
NgManagerBaseURL: "",
104+
BaseURL: "https://app.harness.io",
105+
NgManagerSecret: "test-secret",
106+
},
107+
expectError: false, // The license client creation doesn't validate empty URLs
108+
},
109+
{
110+
name: "empty BaseURL",
111+
config: &config.Config{
112+
AccountID: "test-account",
113+
NgManagerBaseURL: "https://app.harness.io",
114+
BaseURL: "",
115+
NgManagerSecret: "test-secret",
116+
},
117+
expectError: false, // The license client creation doesn't validate empty URLs
118+
},
119+
{
120+
name: "empty NgManagerSecret",
121+
config: &config.Config{
122+
AccountID: "test-account",
123+
NgManagerBaseURL: "https://app.harness.io",
124+
BaseURL: "https://app.harness.io",
125+
NgManagerSecret: "",
126+
},
127+
expectError: false, // The license client creation doesn't validate empty secrets
128+
},
129+
{
130+
name: "nil config",
131+
config: nil,
132+
expectError: true, // This should cause a panic/error when accessing config fields
133+
},
134+
}
135+
136+
for _, tt := range tests {
137+
t.Run(tt.name, func(t *testing.T) {
138+
logger := slog.New(slog.NewTextHandler(os.Stdout, nil))
139+
factory := NewClientFactory(tt.config, logger)
140+
141+
ctx := context.Background()
142+
143+
if tt.expectError {
144+
// For nil config, we expect a panic or error
145+
assert.Panics(t, func() {
146+
client, err := factory.CreateLicenseClient(ctx)
147+
_ = client
148+
_ = err
149+
})
150+
} else {
151+
client, err := factory.CreateLicenseClient(ctx)
152+
assert.NoError(t, err)
153+
assert.NotNil(t, client)
154+
}
155+
})
156+
}
157+
}
158+
159+
func TestClientFactory_CreateLicenseClient_WithContext(t *testing.T) {
160+
config := &config.Config{
161+
AccountID: "test-account",
162+
NgManagerBaseURL: "https://app.harness.io",
163+
BaseURL: "https://app.harness.io",
164+
NgManagerSecret: "test-secret",
165+
}
166+
167+
logger := slog.New(slog.NewTextHandler(os.Stdout, nil))
168+
factory := NewClientFactory(config, logger)
169+
170+
t.Run("context with timeout", func(t *testing.T) {
171+
ctx, cancel := context.WithCancel(context.Background())
172+
defer cancel()
173+
174+
client, err := factory.CreateLicenseClient(ctx)
175+
assert.NoError(t, err)
176+
assert.NotNil(t, client)
177+
})
178+
179+
t.Run("cancelled context", func(t *testing.T) {
180+
ctx, cancel := context.WithCancel(context.Background())
181+
cancel() // Cancel immediately
182+
183+
// The license client creation itself doesn't check context cancellation
184+
// but we test that it doesn't panic with cancelled context
185+
client, err := factory.CreateLicenseClient(ctx)
186+
// This should still succeed as the context is only used for the HTTP client creation
187+
assert.NoError(t, err)
188+
assert.NotNil(t, client)
189+
})
190+
}
191+
192+
func TestClientFactory_Consistency(t *testing.T) {
193+
// Test that multiple calls with same parameters return equivalent clients
194+
config := &config.Config{
195+
AccountID: "test-account",
196+
NgManagerBaseURL: "https://app.harness.io",
197+
BaseURL: "https://app.harness.io",
198+
NgManagerSecret: "test-secret",
199+
}
200+
201+
logger := slog.New(slog.NewTextHandler(os.Stdout, nil))
202+
factory := NewClientFactory(config, logger)
203+
204+
ctx := context.Background()
205+
206+
client1, err1 := factory.CreateLicenseClient(ctx)
207+
require.NoError(t, err1)
208+
require.NotNil(t, client1)
209+
210+
client2, err2 := factory.CreateLicenseClient(ctx)
211+
require.NoError(t, err2)
212+
require.NotNil(t, client2)
213+
214+
// Both clients should be created successfully
215+
// Note: The factory reuses the same client instance for efficiency
216+
// so both calls should return the same instance
217+
assert.Same(t, client1, client2, "Should reuse the same client instance for efficiency")
218+
}
219+
220+
func TestClientFactory_ThreadSafety(t *testing.T) {
221+
// Test concurrent access to the factory
222+
config := &config.Config{
223+
AccountID: "test-account",
224+
NgManagerBaseURL: "https://app.harness.io",
225+
BaseURL: "https://app.harness.io",
226+
NgManagerSecret: "test-secret",
227+
}
228+
229+
logger := slog.New(slog.NewTextHandler(os.Stdout, nil))
230+
factory := NewClientFactory(config, logger)
231+
232+
const numGoroutines = 10
233+
results := make(chan error, numGoroutines)
234+
235+
ctx := context.Background()
236+
237+
// Launch multiple goroutines that create license clients concurrently
238+
for i := 0; i < numGoroutines; i++ {
239+
go func() {
240+
client, err := factory.CreateLicenseClient(ctx)
241+
if err != nil {
242+
results <- err
243+
return
244+
}
245+
if client == nil {
246+
results <- assert.AnError
247+
return
248+
}
249+
results <- nil
250+
}()
251+
}
252+
253+
// Collect results
254+
for i := 0; i < numGoroutines; i++ {
255+
err := <-results
256+
assert.NoError(t, err, "Concurrent license client creation should not fail")
257+
}
258+
}
259+
260+
// Benchmark tests
261+
func BenchmarkClientFactory_CreateLicenseClient(b *testing.B) {
262+
config := &config.Config{
263+
AccountID: "test-account",
264+
NgManagerBaseURL: "https://app.harness.io",
265+
BaseURL: "https://app.harness.io",
266+
NgManagerSecret: "test-secret",
267+
}
268+
269+
logger := slog.New(slog.NewTextHandler(os.Stdout, nil))
270+
factory := NewClientFactory(config, logger)
271+
ctx := context.Background()
272+
273+
b.ResetTimer()
274+
for i := 0; i < b.N; i++ {
275+
client, err := factory.CreateLicenseClient(ctx)
276+
if err != nil {
277+
b.Fatal(err)
278+
}
279+
if client == nil {
280+
b.Fatal("client is nil")
281+
}
282+
}
283+
}

0 commit comments

Comments
 (0)