Skip to content

Commit 5a11e60

Browse files
deansheatherdeansheather
committed
refactor: address PR feedback
- Add type relationship comments between codersdk and tallymansdk types - Move NewOptions type definition above New function in tallymansdk - Rename PublisherWithIngestURL to PublisherWithTallymanBaseURL - Change fakeServer to return *url.URL instead of string - Rename ingestURL to baseURL throughout tests - Add validation for Metronome color names in API - Use real Metronome color names in tests (Primary_medium, UsageLine_0, etc.) - Add test for invalid color name rejection
1 parent 17d391f commit 5a11e60

File tree

7 files changed

+106
-43
lines changed

7 files changed

+106
-43
lines changed

‎codersdk/licenses.go‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -136,6 +136,7 @@ func (c *Client) DeleteLicense(ctx context.Context, id int32) error {
136136
}
137137

138138
// UsageEmbeddableDashboardType represents the type of usage dashboard to embed.
139+
// Corresponds to tallymansdk.DashboardType.
139140
type UsageEmbeddableDashboardType string
140141

141142
const (
@@ -144,18 +145,21 @@ const (
144145
)
145146

146147
// GetUsageEmbeddableDashboardRequest is a request to get an embeddable dashboard URL.
148+
// Corresponds to tallymansdk.RetrieveEmbeddableDashboardRequest.
147149
type GetUsageEmbeddableDashboardRequest struct {
148150
Dashboard UsageEmbeddableDashboardType `json:"dashboard"`
149151
ColorOverrides []DashboardColorOverride `json:"color_overrides,omitempty"`
150152
}
151153

152154
// DashboardColorOverride represents a color override for a dashboard.
155+
// Corresponds to tallymansdk.DashboardColorOverride.
153156
type DashboardColorOverride struct {
154157
Name string `json:"name"`
155158
Value string `json:"value"`
156159
}
157160

158161
// GetUsageEmbeddableDashboardResponse contains the embeddable dashboard URL.
162+
// Corresponds to tallymansdk.RetrieveEmbeddableDashboardResponse.
159163
type GetUsageEmbeddableDashboardResponse struct {
160164
DashboardURL string `json:"dashboard_url"`
161165
}

‎enterprise/coderd/licenses.go‎

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -417,6 +417,40 @@ func (api *API) postUsageEmbeddableDashboard(rw http.ResponseWriter, r *http.Req
417417
return
418418
}
419419

420+
// Validate color override names. These must match the colors available in Metronome.
421+
validColors := map[string]struct{}{
422+
"Gray_dark": {},
423+
"Gray_medium": {},
424+
"Gray_light": {},
425+
"Gray_extralight": {},
426+
"White": {},
427+
"Primary_medium": {},
428+
"Primary_light": {},
429+
"UsageLine_0": {},
430+
"UsageLine_1": {},
431+
"UsageLine_2": {},
432+
"UsageLine_3": {},
433+
"UsageLine_4": {},
434+
"UsageLine_5": {},
435+
"UsageLine_6": {},
436+
"UsageLine_7": {},
437+
"UsageLine_8": {},
438+
"UsageLine_9": {},
439+
"Primary_green": {},
440+
"Primary_red": {},
441+
"Progress_bar": {},
442+
"Progress_bar_background": {},
443+
}
444+
for _, override := range req.ColorOverrides {
445+
if _, ok := validColors[override.Name]; !ok {
446+
httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{
447+
Message: "Invalid color name.",
448+
Detail: fmt.Sprintf("Color name %q is not a valid Metronome color", override.Name),
449+
})
450+
return
451+
}
452+
}
453+
420454
// Create tallyman client
421455
deploymentID, err := uuid.Parse(api.AGPL.DeploymentID)
422456
if err != nil {

‎enterprise/coderd/licenses_test.go‎

Lines changed: 29 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -163,7 +163,7 @@ func TestPostUsageEmbeddableDashboard(t *testing.T) {
163163

164164
// Verify color overrides were passed through
165165
assert.Len(t, req.ColorOverrides, 2)
166-
assert.Equal(t, "primary", req.ColorOverrides[0].Name)
166+
assert.Equal(t, "Primary_medium", req.ColorOverrides[0].Name)
167167
assert.Equal(t, "#FF5733", req.ColorOverrides[0].Value)
168168

169169
w.Header().Set("Content-Type", "application/json")
@@ -192,15 +192,41 @@ func TestPostUsageEmbeddableDashboard(t *testing.T) {
192192
resp, err := client.GetUsageEmbeddableDashboard(ctx, codersdk.GetUsageEmbeddableDashboardRequest{
193193
Dashboard: codersdk.UsageEmbeddableDashboardTypeUsage,
194194
ColorOverrides: []codersdk.DashboardColorOverride{
195-
{Name: "primary", Value: "#FF5733"},
196-
{Name: "secondary", Value: "#33FF57"},
195+
{Name: "Primary_medium", Value: "#FF5733"},
196+
{Name: "UsageLine_0", Value: "#33FF57"},
197197
},
198198
})
199199

200200
require.NoError(t, err)
201201
assert.Equal(t, "https://dashboard.metronome.com/embed/test", resp.DashboardURL)
202202
})
203203

204+
t.Run("InvalidColorNameRejected", func(t *testing.T) {
205+
t.Parallel()
206+
207+
client, _ := coderdenttest.New(t, &coderdenttest.Options{})
208+
coderdenttest.AddLicense(t, client, coderdenttest.LicenseOptions{
209+
AccountType: license.AccountTypeSalesforce,
210+
AccountID: "test-account",
211+
PublishUsageData: true,
212+
})
213+
214+
ctx := testutil.Context(t, testutil.WaitShort)
215+
//nolint:gocritic // owner user is required to read licenses
216+
_, err := client.GetUsageEmbeddableDashboard(ctx, codersdk.GetUsageEmbeddableDashboardRequest{
217+
Dashboard: codersdk.UsageEmbeddableDashboardTypeUsage,
218+
ColorOverrides: []codersdk.DashboardColorOverride{
219+
{Name: "invalid_color_name", Value: "#FF5733"},
220+
},
221+
})
222+
223+
require.Error(t, err)
224+
var sdkErr *codersdk.Error
225+
require.ErrorAs(t, err, &sdkErr)
226+
assert.Equal(t, http.StatusBadRequest, sdkErr.StatusCode())
227+
assert.Contains(t, sdkErr.Message, "Invalid color name")
228+
})
229+
204230
t.Run("UntrustedHostRejected", func(t *testing.T) {
205231
t.Parallel()
206232

‎enterprise/coderd/usage/publisher.go‎

Lines changed: 4 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -101,19 +101,11 @@ func PublisherWithClock(clock quartz.Clock) TallymanPublisherOption {
101101
}
102102
}
103103

104-
// PublisherWithIngestURL sets the ingest URL to use for publishing usage
105-
// events. The base URL is extracted from the ingest URL.
106-
func PublisherWithIngestURL(ingestURL string) TallymanPublisherOption {
104+
// PublisherWithTallymanBaseURL sets the base URL to use for publishing usage
105+
// events.
106+
func PublisherWithTallymanBaseURL(baseURL *url.URL) TallymanPublisherOption {
107107
return func(p *tallymanPublisher) {
108-
parsed, err := url.Parse(ingestURL)
109-
if err != nil {
110-
// This shouldn't happen in practice, but if it does, keep the default.
111-
return
112-
}
113-
p.baseURL = &url.URL{
114-
Scheme: parsed.Scheme,
115-
Host: parsed.Host,
116-
}
108+
p.baseURL = baseURL
117109
}
118110
}
119111

‎enterprise/coderd/usage/publisher_test.go‎

Lines changed: 17 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ import (
66
"encoding/json"
77
"net/http"
88
"net/http/httptest"
9+
"net/url"
910
"testing"
1011
"time"
1112

@@ -53,7 +54,7 @@ func TestIntegration(t *testing.T) {
5354
calls int
5455
handler func(req usagetypes.TallymanV1IngestRequest) any
5556
)
56-
ingestURL := fakeServer(t, tallymanHandler(t, deploymentID.String(), licenseJWT, func(req usagetypes.TallymanV1IngestRequest) any {
57+
baseURL := fakeServer(t, tallymanHandler(t, deploymentID.String(), licenseJWT, func(req usagetypes.TallymanV1IngestRequest) any {
5758
calls++
5859
t.Logf("tallyman backend received call %d", calls)
5960

@@ -89,7 +90,7 @@ func TestIntegration(t *testing.T) {
8990
authzDB := dbauthz.New(db, rbac.NewAuthorizer(prometheus.NewRegistry()), log, coderdtest.AccessControlStorePointer())
9091
publisher := usage.NewTallymanPublisher(ctx, log, authzDB, coderdenttest.Keys,
9192
usage.PublisherWithClock(clock),
92-
usage.PublisherWithIngestURL(ingestURL),
93+
usage.PublisherWithTallymanBaseURL(baseURL),
9394
)
9495
defer publisher.Close()
9596

@@ -210,7 +211,7 @@ func TestPublisherNoEligibleLicenses(t *testing.T) {
210211
db.EXPECT().GetDeploymentID(gomock.Any()).Return(deploymentID.String(), nil).Times(1)
211212

212213
var calls int
213-
ingestURL := fakeServer(t, tallymanHandler(t, deploymentID.String(), "", func(req usagetypes.TallymanV1IngestRequest) any {
214+
baseURL := fakeServer(t, tallymanHandler(t, deploymentID.String(), "", func(req usagetypes.TallymanV1IngestRequest) any {
214215
calls++
215216
return usagetypes.TallymanV1IngestResponse{
216217
AcceptedEvents: []usagetypes.TallymanV1IngestAcceptedEvent{},
@@ -220,7 +221,7 @@ func TestPublisherNoEligibleLicenses(t *testing.T) {
220221

221222
publisher := usage.NewTallymanPublisher(ctx, log, db, coderdenttest.Keys,
222223
usage.PublisherWithClock(clock),
223-
usage.PublisherWithIngestURL(ingestURL),
224+
usage.PublisherWithTallymanBaseURL(baseURL),
224225
)
225226
defer publisher.Close()
226227

@@ -283,7 +284,7 @@ func TestPublisherClaimExpiry(t *testing.T) {
283284
now := time.Now()
284285

285286
var calls int
286-
ingestURL := fakeServer(t, tallymanHandler(t, deploymentID.String(), licenseJWT, func(req usagetypes.TallymanV1IngestRequest) any {
287+
baseURL := fakeServer(t, tallymanHandler(t, deploymentID.String(), licenseJWT, func(req usagetypes.TallymanV1IngestRequest) any {
287288
calls++
288289
return tallymanAcceptAllHandler(req)
289290
}))
@@ -294,7 +295,7 @@ func TestPublisherClaimExpiry(t *testing.T) {
294295

295296
publisher := usage.NewTallymanPublisher(ctx, log, db, coderdenttest.Keys,
296297
usage.PublisherWithClock(clock),
297-
usage.PublisherWithIngestURL(ingestURL),
298+
usage.PublisherWithTallymanBaseURL(baseURL),
298299
usage.PublisherWithInitialDelay(17*time.Minute),
299300
)
300301
defer publisher.Close()
@@ -363,7 +364,7 @@ func TestPublisherMissingEvents(t *testing.T) {
363364
clock.Set(now)
364365

365366
var calls int
366-
ingestURL := fakeServer(t, tallymanHandler(t, deploymentID.String(), licenseJWT, func(req usagetypes.TallymanV1IngestRequest) any {
367+
baseURL := fakeServer(t, tallymanHandler(t, deploymentID.String(), licenseJWT, func(req usagetypes.TallymanV1IngestRequest) any {
367368
calls++
368369
return usagetypes.TallymanV1IngestResponse{
369370
AcceptedEvents: []usagetypes.TallymanV1IngestAcceptedEvent{},
@@ -373,7 +374,7 @@ func TestPublisherMissingEvents(t *testing.T) {
373374

374375
publisher := usage.NewTallymanPublisher(ctx, log, db, coderdenttest.Keys,
375376
usage.PublisherWithClock(clock),
376-
usage.PublisherWithIngestURL(ingestURL),
377+
usage.PublisherWithTallymanBaseURL(baseURL),
377378
)
378379

379380
// Expect the publisher to call SelectUsageEventsForPublishing, followed by
@@ -507,14 +508,14 @@ func TestPublisherLicenseSelection(t *testing.T) {
507508
}, nil)
508509

509510
called := false
510-
ingestURL := fakeServer(t, tallymanHandler(t, deploymentID.String(), expectedLicense, func(req usagetypes.TallymanV1IngestRequest) any {
511+
baseURL := fakeServer(t, tallymanHandler(t, deploymentID.String(), expectedLicense, func(req usagetypes.TallymanV1IngestRequest) any {
511512
called = true
512513
return tallymanAcceptAllHandler(req)
513514
}))
514515

515516
publisher := usage.NewTallymanPublisher(ctx, log, db, coderdenttest.Keys,
516517
usage.PublisherWithClock(clock),
517-
usage.PublisherWithIngestURL(ingestURL),
518+
usage.PublisherWithTallymanBaseURL(baseURL),
518519
)
519520
defer publisher.Close()
520521

@@ -573,7 +574,7 @@ func TestPublisherTallymanError(t *testing.T) {
573574
deploymentID, licenseJWT := configureMockDeployment(t, db)
574575
const errorMessage = "tallyman error"
575576
var calls int
576-
ingestURL := fakeServer(t, tallymanHandler(t, deploymentID.String(), licenseJWT, func(req usagetypes.TallymanV1IngestRequest) any {
577+
baseURL := fakeServer(t, tallymanHandler(t, deploymentID.String(), licenseJWT, func(req usagetypes.TallymanV1IngestRequest) any {
577578
calls++
578579
return usagetypes.TallymanV1Response{
579580
Message: errorMessage,
@@ -582,7 +583,7 @@ func TestPublisherTallymanError(t *testing.T) {
582583

583584
publisher := usage.NewTallymanPublisher(ctx, log, db, coderdenttest.Keys,
584585
usage.PublisherWithClock(clock),
585-
usage.PublisherWithIngestURL(ingestURL),
586+
usage.PublisherWithTallymanBaseURL(baseURL),
586587
)
587588
defer publisher.Close()
588589

@@ -679,11 +680,13 @@ func configureMockDeployment(t *testing.T, db *dbmock.MockStore) (uuid.UUID, str
679680
return deploymentID, licenseRaw
680681
}
681682

682-
func fakeServer(t *testing.T, handler http.Handler) string {
683+
func fakeServer(t *testing.T, handler http.Handler) *url.URL {
683684
t.Helper()
684685
server := httptest.NewServer(handler)
685686
t.Cleanup(server.Close)
686-
return server.URL
687+
baseURL, err := url.Parse(server.URL)
688+
require.NoError(t, err)
689+
return baseURL
687690
}
688691

689692
func tallymanHandler(t *testing.T, expectDeploymentID string, expectLicenseJWT string, handler func(req usagetypes.TallymanV1IngestRequest) any) http.Handler {

‎enterprise/coderd/usage/tallymansdk/client.go‎

Lines changed: 14 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -25,20 +25,6 @@ const (
2525

2626
var ErrNoLicenseSupportsPublishing = xerrors.New("usage publishing is not enabled by any license")
2727

28-
// NewOptions contains options for creating a new Tallyman client.
29-
type NewOptions struct {
30-
// DB is the database store for querying licenses and deployment ID.
31-
DB database.Store
32-
// DeploymentID is the deployment ID. If uuid.Nil, it will be fetched from the database.
33-
DeploymentID uuid.UUID
34-
// LicenseKeys is a map of license keys for verifying license JWTs.
35-
LicenseKeys map[string]ed25519.PublicKey
36-
// BaseURL is the base URL for the Tallyman API. If nil, DefaultURL is used.
37-
BaseURL *url.URL
38-
// HTTPClient is the HTTP client to use for requests. If nil, http.DefaultClient is used.
39-
HTTPClient *http.Client
40-
}
41-
4228
// Client is a client for the Tallyman API.
4329
type Client struct {
4430
// URL is the base URL for the Tallyman API.
@@ -83,6 +69,20 @@ func WithHTTPClient(httpClient *http.Client) ClientOption {
8369
}
8470
}
8571

72+
// NewOptions contains options for creating a new Tallyman client.
73+
type NewOptions struct {
74+
// DB is the database store for querying licenses and deployment ID.
75+
DB database.Store
76+
// DeploymentID is the deployment ID. If uuid.Nil, it will be fetched from the database.
77+
DeploymentID uuid.UUID
78+
// LicenseKeys is a map of license keys for verifying license JWTs.
79+
LicenseKeys map[string]ed25519.PublicKey
80+
// BaseURL is the base URL for the Tallyman API. If nil, DefaultURL is used.
81+
BaseURL *url.URL
82+
// HTTPClient is the HTTP client to use for requests. If nil, http.DefaultClient is used.
83+
HTTPClient *http.Client
84+
}
85+
8686
// New creates a new Tallyman API client by looking up the best license from the database.
8787
// It selects the most recently issued license that:
8888
// - Is unexpired

‎enterprise/coderd/usage/tallymansdk/dashboards.go‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@ import (
99
)
1010

1111
// DashboardType represents the type of dashboard to embed.
12+
// Corresponds to codersdk.UsageEmbeddableDashboardType.
1213
type DashboardType string
1314

1415
const (
@@ -17,18 +18,21 @@ const (
1718
)
1819

1920
// DashboardColorOverride represents a color override for a dashboard.
21+
// Corresponds to codersdk.DashboardColorOverride.
2022
type DashboardColorOverride struct {
2123
Name string `json:"name"`
2224
Value string `json:"value"`
2325
}
2426

2527
// RetrieveEmbeddableDashboardRequest is a request to get an embed URL for a dashboard.
28+
// Corresponds to codersdk.GetUsageEmbeddableDashboardRequest.
2629
type RetrieveEmbeddableDashboardRequest struct {
2730
Dashboard DashboardType `json:"dashboard"`
2831
ColorOverrides []DashboardColorOverride `json:"color_overrides,omitempty"`
2932
}
3033

3134
// RetrieveEmbeddableDashboardResponse is a response containing a dashboard embed URL.
35+
// Corresponds to codersdk.GetUsageEmbeddableDashboardResponse.
3236
type RetrieveEmbeddableDashboardResponse struct {
3337
DashboardURL string `json:"dashboard_url"`
3438
}

0 commit comments

Comments
 (0)