Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 13 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -304,9 +304,22 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/).
longer compiles; keyed literals are unaffected. The field is true when a
provider adopted a commitment that already existed for the same idempotency
token (#211)
- **Breaking (source):** `common.PurchaseResult` gained an `OfferingClass` field.
Downstream code that builds it with unkeyed composite literals no longer
compiles; keyed literals are unaffected. On an adopted re-drive it carries the
offering class AWS reports for the existing RI (the EC2 provider sets it in a
follow-up change); empty otherwise (#211).

### Fixed

- **`common`:** a `Recommendation`, and a `PurchaseResult` carrying one (with
no `Error`), now decodes from JSON when `details` holds an object. `details`
is decoded into the typed pointer for the recommendation's service through
`DecodeServiceDetailsFor`; absent `details` leave the field unchanged and
null sets it nil, as encoding/json does, and details for a service with no
typed details fail with `ErrUntypedServiceDetails`. Before this,
encoding/json could not decode an object into the `ServiceDetails` interface
(#211).
- **Breaking (source):** `managedredis.ReservationsDetailsPager` and `(*managedredis.Client).SetReservationsPager` are removed; tests inject reservation inventory with `SetInventoryFactories` instead (#699).
- **`azure` Redis:** `managedredis.GetExistingCommitments` now reads held reservations from the Reservations API (purchase date, quantity, location, state), so a Redis reservation bought in the last day reduces the matching recommendation instead of being invisible to the duplicate guard (#699). An incomplete live reservation row (missing SKU, location, quantity or purchase date, an unattributable scope, or an expiry falling today) now fails the lookup with an error instead of being skipped, as `azure` compute already does.
- **`aws` RI recommendations:** a fractional Cost Explorer quantity now rounds
Expand Down
29 changes: 29 additions & 0 deletions pkg/common/service_details_codec.go
Original file line number Diff line number Diff line change
Expand Up @@ -172,3 +172,32 @@ func newDetailsForService(service string) (ServiceDetails, bool) {
}
return nil, false
}

// UnmarshalJSON decodes details into the typed pointer for the
// recommendation's service (DecodeServiceDetailsFor), so a Recommendation, and
// a PurchaseResult carrying one, round-trips through JSON. encoding/json cannot
// decode an object into the ServiceDetails interface on its own. As with
// encoding/json, absent details leave Details unchanged and null sets it nil.
func (r *Recommendation) UnmarshalJSON(data []byte) error {
type plain Recommendation
aux := struct {
*plain
Details json.RawMessage `json:"details,omitempty"`
}{plain: (*plain)(r)}
if err := json.Unmarshal(data, &aux); err != nil {
return err
}
if len(aux.Details) == 0 {
return nil
}
if bytes.Equal(aux.Details, jsonNullBytes) {
r.Details = nil
return nil
}
details, err := DecodeServiceDetailsFor(string(r.Service), aux.Details)
if err != nil {
return err
}
r.Details = details
return nil
}
63 changes: 63 additions & 0 deletions pkg/common/service_details_codec_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -310,3 +310,66 @@ func TestDecodeServiceDetailsFor_EveryEmittedPair(t *testing.T) {
})
}
}

func TestRecommendation_DetailsJSONRoundTrip(t *testing.T) {
t.Parallel()
for _, tt := range []struct {
name string
rec Recommendation
details ServiceDetails
}{
{"ec2 compute", Recommendation{Service: ServiceEC2, Details: &ComputeDetails{InstanceType: "m5.large", Platform: "Linux/UNIX", Tenancy: "default", Scope: "Region"}}, &ComputeDetails{InstanceType: "m5.large", Platform: "Linux/UNIX", Tenancy: "default", Scope: "Region"}},
{"value compute decodes to a pointer", Recommendation{Service: ServiceCompute, Details: ComputeDetails{InstanceType: "n2-standard-4"}}, &ComputeDetails{InstanceType: "n2-standard-4"}},
{"rds database", Recommendation{Service: ServiceRDS, Details: &DatabaseDetails{Engine: "postgres", AZConfig: "multi-az", InstanceClass: "db.r6g.large"}}, &DatabaseDetails{Engine: "postgres", AZConfig: "multi-az", InstanceClass: "db.r6g.large"}},
{"no details stays nil", Recommendation{Service: ServiceEC2}, nil},
} {
t.Run(tt.name, func(t *testing.T) {
t.Parallel()
payload, err := json.Marshal(tt.rec)
require.NoError(t, err)
var got Recommendation
require.NoError(t, json.Unmarshal(payload, &got))
assert.Equal(t, tt.details, got.Details)
assert.Equal(t, tt.rec.Service, got.Service)
})
}

t.Run("explicit null details stays nil", func(t *testing.T) {
t.Parallel()
var got Recommendation
require.NoError(t, json.Unmarshal([]byte(`{"service":"ec2","details":null}`), &got))
assert.Nil(t, got.Details)
})

t.Run("decoding into a filled recommendation follows encoding/json", func(t *testing.T) {
t.Parallel()
prior := &ComputeDetails{InstanceType: "m5.large"}
absent := Recommendation{Service: ServiceEC2, Details: prior}
require.NoError(t, json.Unmarshal([]byte(`{"count":2}`), &absent))
assert.Same(t, prior, absent.Details, "absent details leave Details unchanged")
null := Recommendation{Service: ServiceEC2, Details: prior}
require.NoError(t, json.Unmarshal([]byte(`{"details":null}`), &null))
assert.Nil(t, null.Details, "null details set Details nil")
})

t.Run("details for a service with no typed details fail", func(t *testing.T) {
t.Parallel()
var got Recommendation
err := json.Unmarshal([]byte(`{"service":"no-such-service","details":{"x":1}}`), &got)
require.ErrorIs(t, err, ErrUntypedServiceDetails)
})

t.Run("purchase result with details keeps its offering class", func(t *testing.T) {
t.Parallel()
in := PurchaseResult{
Success: true, CommitmentID: "ri-1", ExistingCommitment: true, OfferingClass: "convertible",
Recommendation: Recommendation{Service: ServiceEC2, Details: &ComputeDetails{InstanceType: "m5.large"}},
}
payload, err := json.Marshal(in)
require.NoError(t, err)
var got PurchaseResult
require.NoError(t, json.Unmarshal(payload, &got))
assert.Equal(t, "convertible", got.OfferingClass)
assert.Equal(t, &ComputeDetails{InstanceType: "m5.large"}, got.Recommendation.Details)
})
}
19 changes: 15 additions & 4 deletions pkg/common/types.go
Original file line number Diff line number Diff line change
Expand Up @@ -390,19 +390,30 @@ type PurchaseResult struct {
Error error `json:"error,omitempty"`

// Cost is the total upfront charge for the whole purchase (unit price x count).
// Nil means unknown; a non-nil zero means no upfront charge.
// Nil means unknown; a non-nil zero means no upfront charge. On an adopted
// re-drive (ExistingCommitment) the original purchase's charge is not known:
// the AWS RI services (EC2, RDS, ElastiCache, MemoryDB, OpenSearch, Redshift)
// return nil. GCP Compute Engine reports 0 today and returns nil after a
// follow-up change for #211. Azure reports 0 for a monthly billing plan and
// nil for an upfront one, as on a fresh purchase.
Cost *float64 `json:"cost"`

CommitmentID string `json:"commitment_id,omitempty"`
CommitmentID string `json:"commitment_id,omitempty"`

// OfferingClass is, on an adopted re-drive, the offering class AWS reports
// for the existing commitment; empty otherwise. The EC2 provider sets it in
// a follow-up change for #211.
OfferingClass string `json:"offering_class,omitempty"`
Comment thread
cristim marked this conversation as resolved.
Resolved

Recommendation Recommendation `json:"recommendation"`
Success bool `json:"success"`
DryRun bool `json:"dry_run"`

// ExistingCommitment is true when the provider adopted a commitment that
// already existed for the same idempotency token (a re-drive of an earlier
// attempt) instead of creating a new one. Success stays true and
// CommitmentID names the adopted commitment. Cost semantics on adoption are
// unspecified until the provider changes land. False on a fresh purchase.
// CommitmentID names the adopted commitment. See Cost for its value on
// adoption. False on a fresh purchase.
ExistingCommitment bool `json:"existing_commitment,omitempty"`
}

Expand Down
13 changes: 13 additions & 0 deletions pkg/common/types_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,19 @@ func TestPurchaseResult_ExistingCommitmentJSON(t *testing.T) {
assert.True(t, decoded.ExistingCommitment)
}

func TestPurchaseResult_OfferingClassJSON(t *testing.T) {
empty, err := json.Marshal(PurchaseResult{Success: true, CommitmentID: "ri-1"})
require.NoError(t, err)
assert.NotContains(t, string(empty), "offering_class")

set, err := json.Marshal(PurchaseResult{Success: true, CommitmentID: "ri-1", ExistingCommitment: true, OfferingClass: "convertible"})
require.NoError(t, err)
assert.Contains(t, string(set), `"offering_class":"convertible"`)
var decoded PurchaseResult
require.NoError(t, json.Unmarshal(set, &decoded))
assert.Equal(t, "convertible", decoded.OfferingClass)
}

func TestRecommendation_CommitmentCurrencyJSONAndScaling(t *testing.T) {
t.Parallel()
for _, tt := range []struct {
Expand Down
Loading