From af585a1de005da17c1fd70715a8a7d56da9f65c9 Mon Sep 17 00:00:00 2001 From: Vitaly Antonenko Date: Mon, 24 Jun 2024 14:50:13 +0300 Subject: [PATCH] reafactor: many small fixes accross all storage functions. refactor: start using juju/utils/v3 insead v4 chore: fix commentary for full status reasoning test: add unit test for StorageNotFound error chore: fix StorageContraints typo to StorageConstraints. chore: fix resource_application.go imports. chore: add comments to the storageSetRequiresReplace func. chore: Move from ValueSting to Sting in Sprintf chore: misseing error check chore: add returning errors to resp.Diagnostics refactor: move update storage to a separate method refactor: change channel on revision, not to break test in the future. refactor: move storageSetRequiresReplace to separate file fix: unit test for Storage not found (retry logic) fix: golangci-lint small fixes. chore: fix copyright issue. --- docs/resources/machine.md | 2 +- go.mod | 2 - go.sum | 8 +- internal/juju/applications.go | 54 ++-- internal/juju/applications_test.go | 66 +++++ internal/provider/modifer_storageset.go | 103 ++++++++ internal/provider/resource_application.go | 240 ++++++------------ .../provider/resource_application_test.go | 3 +- internal/provider/resource_integration.go | 4 + internal/provider/resource_machine.go | 2 +- 10 files changed, 283 insertions(+), 201 deletions(-) create mode 100644 internal/provider/modifer_storageset.go diff --git a/docs/resources/machine.md b/docs/resources/machine.md index 7667fda5..700a8fb8 100644 --- a/docs/resources/machine.md +++ b/docs/resources/machine.md @@ -32,7 +32,7 @@ resource "juju_machine" "this_machine" { - `base` (String) The operating system to install on the new machine(s). E.g. ubuntu@22.04. - `constraints` (String) Machine constraints that overwrite those available from 'juju get-model-constraints' and provider's defaults. -- `disks` (String) StorageContraints constraints for disks to attach to the machine(s). +- `disks` (String) Storage constraints for disks to attach to the machine(s). - `name` (String) A name for the machine resource in Terraform. - `placement` (String) Additional information about how to allocate the machine in the cloud. - `private_key_file` (String) The file path to read the private key from. diff --git a/go.mod b/go.mod index 422750f0..6cddb4b8 100644 --- a/go.mod +++ b/go.mod @@ -26,7 +26,6 @@ require ( github.com/juju/names/v5 v5.0.0 github.com/juju/retry v1.0.0 github.com/juju/utils/v3 v3.1.1 - github.com/juju/utils/v4 v4.0.2 github.com/juju/version/v2 v2.0.1 github.com/rs/zerolog v1.32.0 github.com/stretchr/testify v1.9.0 @@ -125,7 +124,6 @@ require ( github.com/juju/idmclient/v2 v2.0.0 // indirect github.com/juju/jsonschema v1.0.0 // indirect github.com/juju/loggo v1.0.0 // indirect - github.com/juju/loggo/v2 v2.0.0 // indirect github.com/juju/lru v1.0.0 // indirect github.com/juju/lumberjack/v2 v2.0.2 // indirect github.com/juju/mgo/v3 v3.0.4 // indirect diff --git a/go.sum b/go.sum index 50758a11..5a885330 100644 --- a/go.sum +++ b/go.sum @@ -358,8 +358,6 @@ github.com/juju/juju v0.0.0-20240524040137-95c267441801 h1:5aNKt2VkNKsS2JI2QcCjN github.com/juju/juju v0.0.0-20240524040137-95c267441801/go.mod h1:dxhhMNnbWSXUB7EdQEsaxLhgwKQXo2Ctl4z4AZwWL88= github.com/juju/loggo v1.0.0 h1:Y6ZMQOGR9Aj3BGkiWx7HBbIx6zNwNkxhVNOHU2i1bl0= github.com/juju/loggo v1.0.0/go.mod h1:NIXFioti1SmKAlKNuUwbMenNdef59IF52+ZzuOmHYkg= -github.com/juju/loggo/v2 v2.0.0 h1:PzyVIn+NgoZ22QUtPgKF/lh+6SnaCOEXhcP+sE4FhOk= -github.com/juju/loggo/v2 v2.0.0/go.mod h1:647d6WvXBLj5lvka2qBvccr7vMIvF2KFkEH+0ZuFOUM= github.com/juju/lru v1.0.0 h1:FP8mBNF3jBnKwGO5PtsR+8iIegx8DREfhRhpcGpYcn4= github.com/juju/lru v1.0.0/go.mod h1:YauKGp6PAhOQyGuTOkiFdXVP1jR3vLkp/FI71GcpYcQ= github.com/juju/lumberjack/v2 v2.0.2 h1:FlxrR62Vb7FfN7jwpSBqqereyq5bBQUF0LqOhZ2VGeI= @@ -402,16 +400,14 @@ github.com/juju/rpcreflect v1.2.0/go.mod h1:yWSyMkWOwQGqUVxvboHn1c3CuCQrQ5LgV//8 github.com/juju/schema v1.0.0/go.mod h1:Y+ThzXpUJ0E7NYYocAbuvJ7vTivXfrof/IfRPq/0abI= github.com/juju/schema v1.2.0 h1:+XywM0pYzuhGebQiK1aR4Bj7Q7nLV5MihcOgq6dLLxs= github.com/juju/schema v1.2.0/go.mod h1:VdljuJLc45loM79LYm4yKKmPJwK1bPKRekvMVlfywU0= -github.com/juju/testing v1.2.0 h1:Q0wxjaxx4XPVEN+SgzxKr3d82pjmSBcuM3WndAU391c= -github.com/juju/testing v1.2.0/go.mod h1:lqZVzNwBKAbylGZidK77ts6kIdoOkmD52+4m0ysetPo= +github.com/juju/testing v1.1.0 h1:+WWez0vCu6dtnpLIzfuuo3bN3x62LBIyMDCfvMYP+Qg= +github.com/juju/testing v1.1.0/go.mod h1:1XQGptw6JWFvRWb3ewilUdTBG0oGcoI2kdX9Z1VEzhU= github.com/juju/txn/v3 v3.0.2 h1:ibKhRlQmslPj88QfxND8hX4Sv6rTRKOb+HSp/ti1sEg= github.com/juju/txn/v3 v3.0.2/go.mod h1:DlFlqNZkgzE4NolIxhSvYOok/heIOjhXLx3Z5oUTyy4= github.com/juju/usso v1.0.1 h1:zyQhSUJnhFZdPqVAmPeqXYlnYXv+i0Cp1Ii+aziMXGs= github.com/juju/usso v1.0.1/go.mod h1:3cvBcGVmWXyHhrBHBQtpNBzca/JRg4S5XH88Hj/NsYA= github.com/juju/utils/v3 v3.1.1 h1:shEMr/4Wkw0YCOPz5IFOYkLv1ec50pzRi59TRl0qQ/0= github.com/juju/utils/v3 v3.1.1/go.mod h1:nAj3sHtdYfAkvnkqttTy3Xzm2HzkD9Hfgnc+upOW2Z8= -github.com/juju/utils/v4 v4.0.2 h1:lh1cPruYCPLWBLuZpaAP18cc+3j9X/JsLExjwQ/+NxQ= -github.com/juju/utils/v4 v4.0.2/go.mod h1:j5wVHbRzw2LF85mb3H46cPPXBkyw5k4laDL6cOW55LY= github.com/juju/version/v2 v2.0.1 h1:4psP9XDv8qIbSdv3llCmcmBXe+wvm57BvYp2sG/i6zc= github.com/juju/version/v2 v2.0.1/go.mod h1:OK+DKO9ve3fFCVUZGT8ZbUWU2TnZh914S6gMhiM2hXg= github.com/juju/webbrowser v1.0.0 h1:JLdmbFtCGY6Qf2jmS6bVaenJFGIFkdF1/BjUm76af78= diff --git a/internal/juju/applications.go b/internal/juju/applications.go index 3983cbdf..20900c4e 100644 --- a/internal/juju/applications.go +++ b/internal/juju/applications.go @@ -137,22 +137,22 @@ func ConfigEntryToString(input interface{}) string { } type CreateApplicationInput struct { - ApplicationName string - ModelName string - CharmName string - CharmChannel string - CharmBase string - CharmSeries string - CharmRevision int - Units int - Trust bool - Expose map[string]interface{} - Config map[string]string - Placement string - Constraints constraints.Value - EndpointBindings map[string]string - Resources map[string]int - StorageContraints map[string]jujustorage.Constraints + ApplicationName string + ModelName string + CharmName string + CharmChannel string + CharmBase string + CharmSeries string + CharmRevision int + Units int + Trust bool + Expose map[string]interface{} + Config map[string]string + Placement string + Constraints constraints.Value + EndpointBindings map[string]string + Resources map[string]int + StorageConstraints map[string]jujustorage.Constraints } // validateAndTransform returns transformedCreateApplicationInput which @@ -169,7 +169,7 @@ func (input CreateApplicationInput) validateAndTransform(conn api.Connection) (p parsed.trust = input.Trust parsed.units = input.Units parsed.resources = input.Resources - parsed.storage = input.StorageContraints + parsed.storage = input.StorageConstraints appName := input.ApplicationName if appName == "" { @@ -781,7 +781,7 @@ func (c applicationsClient) ReadApplicationWithRetryOnNotFound(ctx context.Conte return fmt.Errorf("ReadApplicationWithRetryOnNotFound: need %d machines, have %d", output.Units, len(machines)) } - // NOTE: Application can always have storages. However, they + // NOTE: Application can always have storage. However, they // will not be listed right after the application is created. So // we need to wait for the storages to be ready. And we need to // check if all storage constraints have pool equal "" and size equal 0 @@ -892,8 +892,10 @@ func (c applicationsClient) ReadApplication(input *ReadApplicationInput) (*ReadA appInfo := apps[0].Result - // TODO: Investigate why we're getting the full status here when - // application status is needed. include storage. + // We are currently retrieving the full status, which might be more information than necessary. + // The main focus is on the application status, particularly including the storage status. + // It might be more efficient to directly query for the application status and its associated storage status. + // TODO: Investigate if there's a way to optimize this by only fetching the required information. status, err := clientAPIClient.Status(&apiclient.StatusArgs{ Patterns: []string{}, IncludeStorage: true, @@ -912,20 +914,10 @@ func (c applicationsClient) ReadApplication(input *ReadApplicationInput) (*ReadA return nil, fmt.Errorf("no status returned for application: %s", input.AppName) } - //// Print Full status - //c.Tracef("Full status", map[string]interface{}{"status": status}) - // - //// Get storage info from the full status. - //for _, sst := range status.StorageContraints { - // c.Tracef("StorageContraints status", map[string]interface{}{"storage": sst.StorageTag, "kind": sst.Kind}) - //} - //for _, fst := range status.Filesystems { - // c.Tracef("Filesystem status", map[string]interface{}{"storage": fst.StorageContraints.StorageTag, "pool": fst.Info.Pool, "size": fst.Info.Size}) - //} storages := transformToStorageConstraints(status.Storage, status.Filesystems, status.Volumes) // Print storage to console for k, v := range storages { - c.Tracef("StorageContraints constraints", map[string]interface{}{"storage": k, "constraints": v}) + c.Tracef("StorageConstraints constraints", map[string]interface{}{"storage": k, "constraints": v}) } allocatedMachines := set.NewStrings() diff --git a/internal/juju/applications_test.go b/internal/juju/applications_test.go index 67b56d7c..ec7b996f 100644 --- a/internal/juju/applications_test.go +++ b/internal/juju/applications_test.go @@ -298,6 +298,72 @@ func (s *ApplicationSuite) TestReadApplicationRetrySubordinate() { s.Assert().Equal("ubuntu@22.04", resp.Base) } +// TestReadApplicationRetryNotFoundStorageNotFoundError tests the case where the first response is a storage not found error. +// The second response is a real application. +func (s *ApplicationSuite) TestReadApplicationRetryNotFoundStorageNotFoundError() { + defer s.setupMocks(s.T()).Finish() + s.mockSharedClient.EXPECT().ModelType(gomock.Any()).Return(model.IAAS, nil).AnyTimes() + + appName := "testapplication" + aExp := s.mockApplicationClient.EXPECT() + + // First response is a storage not found error. + aExp.ApplicationsInfo(gomock.Any()).Return([]params.ApplicationInfoResult{{ + Error: ¶ms.Error{Message: `storage "testapplication" not found`, Code: "not found"}, + }}, nil) + + // Retry - expect ApplicationsInfo and Status to be called. + // The second time return a real application. + amdConst := constraints.MustParse("arch=amd64") + infoResult := params.ApplicationInfoResult{ + Result: ¶ms.ApplicationResult{ + Tag: names.NewApplicationTag(appName).String(), + Charm: "ch:amd64/jammy/testcharm-5", + Base: params.Base{Name: "ubuntu", Channel: "22.04"}, + Channel: "stable", + Constraints: amdConst, + Principal: true, + }, + Error: nil, + } + + aExp.ApplicationsInfo(gomock.Any()).Return([]params.ApplicationInfoResult{infoResult}, nil) + getResult := ¶ms.ApplicationGetResults{ + Application: appName, + CharmConfig: nil, + ApplicationConfig: nil, + Charm: "ch:amd64/jammy/testcharm-5", + Base: params.Base{Name: "ubuntu", Channel: "22.04"}, + Channel: "stable", + Constraints: amdConst, + EndpointBindings: nil, + } + aExp.Get("master", appName).Return(getResult, nil) + statusResult := ¶ms.FullStatus{ + Applications: map[string]params.ApplicationStatus{appName: { + Charm: "ch:amd64/jammy/testcharm-5", + Units: map[string]params.UnitStatus{"testapplication/0": { + Machine: "0", + }}, + }}, + } + s.mockClient.EXPECT().Status(gomock.Any()).Return(statusResult, nil) + + client := s.getApplicationsClient() + resp, err := client.ReadApplicationWithRetryOnNotFound(context.Background(), + &ReadApplicationInput{ + ModelName: s.testModelName, + AppName: appName, + }) + s.Require().NoError(err, "error from ReadApplicationWithRetryOnNotFound") + s.Require().NotNil(resp, "ReadApplicationWithRetryOnNotFound response") + + s.Assert().Equal("testcharm", resp.Name) + s.Assert().Equal("stable", resp.Channel) + s.Assert().Equal(5, resp.Revision) + s.Assert().Equal("ubuntu@22.04", resp.Base) +} + // In order for 'go test' to run this suite, we need to create // a normal test function and pass our suite to suite.Run func TestApplicationSuite(t *testing.T) { diff --git a/internal/provider/modifer_storageset.go b/internal/provider/modifer_storageset.go new file mode 100644 index 00000000..d41d86c2 --- /dev/null +++ b/internal/provider/modifer_storageset.go @@ -0,0 +1,103 @@ +// Copyright 2024 Canonical Ltd. +// Licensed under the AGPLv3, see LICENCE file for details. + +package provider + +import ( + "context" + "fmt" + + "github.com/hashicorp/terraform-plugin-framework/resource/schema/planmodifier" + "github.com/hashicorp/terraform-plugin-framework/resource/schema/setplanmodifier" + jujustorage "github.com/juju/juju/storage" + "github.com/juju/utils/v3" +) + +// storageSetRequiresReplace is a plan modifier function that determines if the storage set requires a replace. +// It compares the storage set in the plan with the storage set in the state. +// Return false if new items were added and old items were not changed. +// Return true if old items were removed +func storageSetRequiresReplace(ctx context.Context, req planmodifier.SetRequest, resp *setplanmodifier.RequiresReplaceIfFuncResponse) { + planSet := make(map[string]jujustorage.Constraints) + if !req.PlanValue.IsNull() { + var planStorageSlice []nestedStorage + resp.Diagnostics.Append(req.PlanValue.ElementsAs(ctx, &planStorageSlice, false)...) + if resp.Diagnostics.HasError() { + return + } + if len(planStorageSlice) > 0 { + for _, storage := range planStorageSlice { + storageName := storage.Label.ValueString() + storageSize := storage.Size.ValueString() + storagePool := storage.Pool.ValueString() + storageCount := storage.Count.ValueInt64() + + // Validate storage size + parsedStorageSize, err := utils.ParseSize(storageSize) + if err != nil { + resp.Diagnostics.AddError("Invalid Storage Size", fmt.Sprintf("Invalid storage size %q: %s", storageSize, err)) + return + } + + planSet[storageName] = jujustorage.Constraints{ + Size: parsedStorageSize, + Pool: storagePool, + Count: uint64(storageCount), + } + } + } + } + + stateSet := make(map[string]jujustorage.Constraints) + if !req.StateValue.IsNull() { + var stateStorageSlice []nestedStorage + resp.Diagnostics.Append(req.StateValue.ElementsAs(ctx, &stateStorageSlice, false)...) + if resp.Diagnostics.HasError() { + return + } + if len(stateStorageSlice) > 0 { + for _, storage := range stateStorageSlice { + storageName := storage.Label.ValueString() + storageSize := storage.Size.ValueString() + storagePool := storage.Pool.ValueString() + storageCount := storage.Count.ValueInt64() + + // Validate storage size + parsedStorageSize, err := utils.ParseSize(storageSize) + if err != nil { + resp.Diagnostics.AddError("Invalid Storage Size", fmt.Sprintf("Invalid storage size %q: %s", storageSize, err)) + return + } + + stateSet[storageName] = jujustorage.Constraints{ + Size: parsedStorageSize, + Pool: storagePool, + Count: uint64(storageCount), + } + } + } + } + + // Return false if new items were added and old items were not changed + for key, value := range planSet { + stateValue, ok := stateSet[key] + if !ok { + resp.RequiresReplace = false + return + } + if (value.Size != stateValue.Size) || (value.Pool != stateValue.Pool) || (value.Count != stateValue.Count) { + resp.RequiresReplace = true + return + } + } + + // Return true if old items were removed + for key := range stateSet { + if _, ok := planSet[key]; !ok { + resp.RequiresReplace = true + return + } + } + + resp.RequiresReplace = false +} diff --git a/internal/provider/resource_application.go b/internal/provider/resource_application.go index e9a1d451..d564ab0c 100644 --- a/internal/provider/resource_application.go +++ b/internal/provider/resource_application.go @@ -6,7 +6,6 @@ package provider import ( "context" "fmt" - "github.com/hashicorp/terraform-plugin-framework/resource/schema/setplanmodifier" "strings" "github.com/dustin/go-humanize" @@ -22,6 +21,7 @@ import ( "github.com/hashicorp/terraform-plugin-framework/resource/schema/int64default" "github.com/hashicorp/terraform-plugin-framework/resource/schema/int64planmodifier" "github.com/hashicorp/terraform-plugin-framework/resource/schema/planmodifier" + "github.com/hashicorp/terraform-plugin-framework/resource/schema/setplanmodifier" "github.com/hashicorp/terraform-plugin-framework/resource/schema/stringdefault" "github.com/hashicorp/terraform-plugin-framework/resource/schema/stringplanmodifier" "github.com/hashicorp/terraform-plugin-framework/schema/validator" @@ -29,10 +29,9 @@ import ( "github.com/hashicorp/terraform-plugin-framework/types" "github.com/hashicorp/terraform-plugin-log/tflog" "github.com/juju/errors" - "github.com/juju/utils/v4" - "github.com/juju/juju/core/constraints" jujustorage "github.com/juju/juju/storage" + "github.com/juju/utils/v3" "github.com/juju/terraform-provider-juju/internal/juju" ) @@ -129,92 +128,6 @@ func (r *applicationResource) Configure(ctx context.Context, req resource.Config r.subCtx = tflog.NewSubsystem(ctx, LogResourceApplication) } -func storageSetRequiresReplace(ctx context.Context, req planmodifier.SetRequest, resp *setplanmodifier.RequiresReplaceIfFuncResponse) { - - planSet := make(map[string]jujustorage.Constraints) - if !req.PlanValue.IsNull() { - var planStorageSlice []nestedStorage - resp.Diagnostics.Append(req.PlanValue.ElementsAs(ctx, &planStorageSlice, false)...) - if resp.Diagnostics.HasError() { - return - } - if len(planStorageSlice) > 0 { - for _, storage := range planStorageSlice { - storageName := storage.Label.ValueString() - storageSize := storage.Size.ValueString() - storagePool := storage.Pool.ValueString() - storageCount := storage.Count.ValueInt64() - - // Validate storage size - parsedStorageSize, err := utils.ParseSize(storageSize) - if err != nil { - resp.Diagnostics.AddError("Invalid StorageContraints Size", fmt.Sprintf("Invalid storage size %q: %s", storageSize, err)) - return - } - - planSet[storageName] = jujustorage.Constraints{ - Size: parsedStorageSize, - Pool: storagePool, - Count: uint64(storageCount), - } - } - } - } - - stateSet := make(map[string]jujustorage.Constraints) - if !req.StateValue.IsNull() { - var stateStorageSlice []nestedStorage - resp.Diagnostics.Append(req.StateValue.ElementsAs(ctx, &stateStorageSlice, false)...) - if resp.Diagnostics.HasError() { - return - } - if len(stateStorageSlice) > 0 { - for _, storage := range stateStorageSlice { - storageName := storage.Label.ValueString() - storageSize := storage.Size.ValueString() - storagePool := storage.Pool.ValueString() - storageCount := storage.Count.ValueInt64() - - // Validate storage size - parsedStorageSize, err := utils.ParseSize(storageSize) - if err != nil { - resp.Diagnostics.AddError("Invalid StorageConstraints Size", fmt.Sprintf("Invalid storage size %q: %s", storageSize, err)) - return - } - - stateSet[storageName] = jujustorage.Constraints{ - Size: parsedStorageSize, - Pool: storagePool, - Count: uint64(storageCount), - } - } - } - } - - // Return false if new items were added and old items were not changed - for key, value := range planSet { - stateValue, ok := stateSet[key] - if !ok { - resp.RequiresReplace = false - return - } - if (value.Size != stateValue.Size) || (value.Pool != stateValue.Pool) || (value.Count != stateValue.Count) { - resp.RequiresReplace = true - return - } - } - - // Return true if old items were removed - for key := range stateSet { - if _, ok := planSet[key]; !ok { - resp.RequiresReplace = true - return - } - } - - resp.RequiresReplace = false -} - func (r *applicationResource) Schema(_ context.Context, _ resource.SchemaRequest, resp *resource.SchemaResponse) { resp.Schema = schema.Schema{ Description: "A resource that represents a single Juju application deployment from a charm. Deployment of bundles" + @@ -524,7 +437,7 @@ type nestedStorage struct { } func (n nestedStorage) transformToString() string { - return fmt.Sprintf("%s=%s,%s,%d", n.Label.ValueString(), n.Size.ValueString(), n.Pool.ValueString(), n.Count.ValueInt64()) + return fmt.Sprintf("%q=%q,%q,%d", n.Label.String(), n.Size.String(), n.Pool.String(), n.Count.ValueInt64()) } // Create is called when the provider must create a new resource. Config @@ -641,7 +554,7 @@ func (r *applicationResource) Create(ctx context.Context, req resource.CreateReq // Validate storage size parsedStorageSize, err := utils.ParseSize(storageSize) if err != nil { - resp.Diagnostics.AddError("Invalid StorageContraints Size", fmt.Sprintf("Invalid storage size %q: %s", storageSize, err)) + resp.Diagnostics.AddError("Invalid Storage Size", fmt.Sprintf("Invalid storage size %q: %s", storageSize, err)) return } @@ -657,22 +570,22 @@ func (r *applicationResource) Create(ctx context.Context, req resource.CreateReq modelName := plan.ModelName.ValueString() createResp, err := r.client.Applications.CreateApplication(ctx, &juju.CreateApplicationInput{ - ApplicationName: plan.ApplicationName.ValueString(), - ModelName: modelName, - CharmName: charmName, - CharmChannel: channel, - CharmRevision: revision, - CharmBase: planCharm.Base.ValueString(), - CharmSeries: planCharm.Series.ValueString(), - Units: int(plan.UnitCount.ValueInt64()), - Config: configField, - Constraints: parsedConstraints, - Trust: plan.Trust.ValueBool(), - Expose: expose, - Placement: plan.Placement.ValueString(), - EndpointBindings: endpointBindings, - Resources: resourceRevisions, - StorageContraints: storageConstraints, + ApplicationName: plan.ApplicationName.ValueString(), + ModelName: modelName, + CharmName: charmName, + CharmChannel: channel, + CharmRevision: revision, + CharmBase: planCharm.Base.ValueString(), + CharmSeries: planCharm.Series.ValueString(), + Units: int(plan.UnitCount.ValueInt64()), + Config: configField, + Constraints: parsedConstraints, + Trust: plan.Trust.ValueBool(), + Expose: expose, + Placement: plan.Placement.ValueString(), + EndpointBindings: endpointBindings, + Resources: resourceRevisions, + StorageConstraints: storageConstraints, }, ) if err != nil { @@ -770,8 +683,7 @@ func (r *applicationResource) Read(ctx context.Context, req resource.ReadRequest if response == nil { return } - r.trace(fmt.Sprintf("read application resource %q", appName)) - r.trace("ReadApplication response", map[string]interface{}{"response": response}) + r.trace(fmt.Sprint("read application", map[string]interface{}{"resource": appName, "response": response})) state.ApplicationName = types.StringValue(appName) state.ModelName = types.StringValue(modelName) @@ -846,7 +758,10 @@ func (r *applicationResource) Read(ctx context.Context, req resource.ReadRequest } storageType := req.State.Schema.GetAttributes()[StorageKey].(schema.SetNestedAttribute).NestedObject.Type() state.Storage, dErr = types.SetValueFrom(ctx, storageType, nestedStorageSlice) - + if dErr.HasError() { + resp.Diagnostics.Append(dErr...) + return + } resourceType := req.State.Schema.GetAttributes()[ResourceKey].(schema.MapAttribute).ElementType state.Resources, dErr = r.configureResourceData(ctx, resourceType, state.Resources, response.Resources) if dErr.HasError() { @@ -1086,57 +1001,12 @@ func (r *applicationResource) Update(ctx context.Context, req resource.UpdateReq updateApplicationInput.EndpointBindings = endpointBindings } - // Check if we have new storages in plan that not existed in the state, and add their constraints to the - // update application input + // Check if we have new storage in plan that not existed in the state, and add their constraints to the + // update application input. if !plan.Storage.Equal(state.Storage) { - var planStorageSlice, stateStorageSlice []nestedStorage - resp.Diagnostics.Append(plan.Storage.ElementsAs(ctx, &planStorageSlice, false)...) - if resp.Diagnostics.HasError() { - return - } - resp.Diagnostics.Append(state.Storage.ElementsAs(ctx, &stateStorageSlice, false)...) - if resp.Diagnostics.HasError() { + if r.updateStorage(ctx, resp, plan, state, updateApplicationInput) { return } - - // Map of storage name to storage constraints - planStorageMap := make(map[string]jujustorage.Constraints) - for _, storage := range planStorageSlice { - // Validate storage size - parsedSize, err := utils.ParseSize(storage.Size.ValueString()) - if err != nil { - return - } - planStorageMap[storage.Label.ValueString()] = jujustorage.Constraints{ - Size: parsedSize, - Pool: storage.Pool.ValueString(), - Count: uint64(storage.Count.ValueInt64()), - } - } - stateStorageMap := make(map[string]jujustorage.Constraints) - for _, storage := range stateStorageSlice { - // Validate storage size - parsedSize, err := utils.ParseSize(storage.Size.ValueString()) - if err != nil { - return - } - stateStorageMap[storage.Label.ValueString()] = jujustorage.Constraints{ - Size: parsedSize, - Pool: storage.Pool.ValueString(), - Count: uint64(storage.Count.ValueInt64()), - } - } - - // Create a map of updated storage constraints that are in the plan but not in the state - updatedStorageMap := make(map[string]jujustorage.Constraints) - for _, storage := range planStorageSlice { - if _, ok := stateStorageMap[storage.Label.ValueString()]; !ok { - updatedStorageMap[storage.Label.ValueString()] = planStorageMap[storage.Label.ValueString()] - } - } - - // Update the storage constraints for the application - updateApplicationInput.StorageConstraints = updatedStorageMap } r.trace("update input", map[string]interface{}{"updateApplicationInput": updateApplicationInput}) @@ -1152,6 +1022,60 @@ func (r *applicationResource) Update(ctx context.Context, req resource.UpdateReq resp.Diagnostics.Append(resp.State.Set(ctx, &plan)...) } +func (r *applicationResource) updateStorage(ctx context.Context, resp *resource.UpdateResponse, plan applicationResourceModel, state applicationResourceModel, updateApplicationInput juju.UpdateApplicationInput) bool { + var planStorageSlice, stateStorageSlice []nestedStorage + resp.Diagnostics.Append(plan.Storage.ElementsAs(ctx, &planStorageSlice, false)...) + if resp.Diagnostics.HasError() { + return true + } + resp.Diagnostics.Append(state.Storage.ElementsAs(ctx, &stateStorageSlice, false)...) + if resp.Diagnostics.HasError() { + return true + } + + // Map of storage name to storage constraints + planStorageMap := make(map[string]jujustorage.Constraints) + for _, storage := range planStorageSlice { + // Validate storage size + parsedSize, err := utils.ParseSize(storage.Size.ValueString()) + if err != nil { + resp.Diagnostics.AddError("Invalid Storage Size", fmt.Sprintf("Invalid storage size %q: %s", storage.Size.ValueString(), err)) + return true + } + planStorageMap[storage.Label.ValueString()] = jujustorage.Constraints{ + Size: parsedSize, + Pool: storage.Pool.ValueString(), + Count: uint64(storage.Count.ValueInt64()), + } + } + stateStorageMap := make(map[string]jujustorage.Constraints) + for _, storage := range stateStorageSlice { + // Validate storage size + parsedSize, err := utils.ParseSize(storage.Size.ValueString()) + if err != nil { + resp.Diagnostics.AddError("Invalid Storage Size", fmt.Sprintf("Invalid storage size %q: %s", storage.Size.ValueString(), err)) + return true + } + stateStorageMap[storage.Label.ValueString()] = jujustorage.Constraints{ + Size: parsedSize, + Pool: storage.Pool.ValueString(), + Count: uint64(storage.Count.ValueInt64()), + } + } + + // Create a map of updated storage constraints that are in the plan but not in the state + updatedStorageMap := make(map[string]jujustorage.Constraints) + for _, storage := range planStorageSlice { + if _, ok := stateStorageMap[storage.Label.ValueString()]; !ok { + updatedStorageMap[storage.Label.ValueString()] = planStorageMap[storage.Label.ValueString()] + } + } + + // Update the storage constraints for the application + updateApplicationInput.StorageConstraints = updatedStorageMap + return false +} + // computeExposeDeltas computes the differences between the previously // stored expose value and the current one. The valueSet argument is used // to indicate whether the value was already set or not in the latest diff --git a/internal/provider/resource_application_test.go b/internal/provider/resource_application_test.go index cdc57ff4..7daf51e2 100644 --- a/internal/provider/resource_application_test.go +++ b/internal/provider/resource_application_test.go @@ -892,7 +892,7 @@ resource "juju_application" "{{.AppName}}" { name = "{{.AppName}}" charm { name = "postgresql" - channel = "latest/stable" + revision = 24 } storage = [{ @@ -907,7 +907,6 @@ resource "juju_application" "{{.AppName}}" { "AppName": appName, "StorageConstraints": storageConstraints, }) - } func testCheckEndpointsAreSetToCorrectSpace(modelName, appName, defaultSpace string, configuredEndpoints map[string]string) resource.TestCheckFunc { diff --git a/internal/provider/resource_integration.go b/internal/provider/resource_integration.go index 99e23881..a55a9ecc 100644 --- a/internal/provider/resource_integration.go +++ b/internal/provider/resource_integration.go @@ -222,6 +222,10 @@ func (r *integrationResource) Create(ctx context.Context, req resource.CreateReq appsType := req.Plan.Schema.GetBlocks()["application"].(schema.SetNestedBlock).NestedObject.Type() parsedApps, errDiag := types.SetValueFrom(ctx, appsType, parsedApplications) + if errDiag.HasError() { + resp.Diagnostics.Append(errDiag...) + return + } resp.Diagnostics.Append(errDiag...) if resp.Diagnostics.HasError() { return diff --git a/internal/provider/resource_machine.go b/internal/provider/resource_machine.go index 29c19232..2d0005d2 100644 --- a/internal/provider/resource_machine.go +++ b/internal/provider/resource_machine.go @@ -128,7 +128,7 @@ func (r *machineResource) Schema(_ context.Context, req resource.SchemaRequest, }, }, DisksKey: schema.StringAttribute{ - Description: "StorageContraints constraints for disks to attach to the machine(s).", + Description: "Storage constraints for disks to attach to the machine(s).", Optional: true, PlanModifiers: []planmodifier.String{ stringplanmodifier.RequiresReplaceIfConfigured(),