From 524ea8f2f31ac1baf6e22cbfddbd7f8180524832 Mon Sep 17 00:00:00 2001 From: Myroslav Vivcharyk Date: Fri, 13 Dec 2024 13:56:37 +0100 Subject: [PATCH] refactor(organizational_unit): replaced client-v2 with avngen (#1940) --- internal/schemautil/helpers.go | 47 +++---- internal/schemautil/helpers_test.go | 85 ++++++++++++ .../organization/organizational_unit.go | 131 +++++++++++------- .../organizational_unit_data_source.go | 26 ++-- .../organization/organizational_unit_test.go | 94 +++++++++++++ .../service/project/billing_group.go | 4 +- .../sdkprovider/service/project/project.go | 2 +- internal/sdkprovider/service/project/util.go | 33 +++++ 8 files changed, 323 insertions(+), 99 deletions(-) create mode 100644 internal/schemautil/helpers_test.go create mode 100644 internal/sdkprovider/service/organization/organizational_unit_test.go diff --git a/internal/schemautil/helpers.go b/internal/schemautil/helpers.go index 0ece353a7..2f5ef0769 100644 --- a/internal/schemautil/helpers.go +++ b/internal/schemautil/helpers.go @@ -6,6 +6,7 @@ import ( "strings" "github.com/aiven/aiven-go-client/v2" + "github.com/aiven/go-client-codegen/handler/organization" "github.com/aiven/go-client-codegen/handler/service" "github.com/docker/go-units" "github.com/hashicorp/go-cty/cty" @@ -137,8 +138,8 @@ func HumanReadableByteSize(s int) string { return units.CustomSize("%.12g%s", float64(s), 1024.0, suffixes) } -// isStringAnOrganizationID is a helper function that returns true if the string is an organization ID. -func isStringAnOrganizationID(s string) bool { +// IsOrganizationID is a helper function that returns true if the string is an organization ID. +func IsOrganizationID(s string) bool { return strings.HasPrefix(s, "org") } @@ -146,7 +147,7 @@ func isStringAnOrganizationID(s string) bool { // If the ID is an organization ID, it will be converted to an account ID via the API. // If the ID is an account ID, it will be returned as is, without performing any API calls. func NormalizeOrganizationID(ctx context.Context, client *aiven.Client, id string) (string, error) { - if isStringAnOrganizationID(id) { + if IsOrganizationID(id) { r, err := client.Organization.Get(ctx, id) if err != nil { return "", err @@ -158,37 +159,23 @@ func NormalizeOrganizationID(ctx context.Context, client *aiven.Client, id strin return id, nil } -// DetermineMixedOrganizationConstraintIDToStore is a helper function that returns the ID to store in the state. -// We have several fields that can be either an organization ID or an account ID. -// We want to store the one that was already in the state, if it was already there. -// If it was not, we want to prioritize the organization ID, but if it is not available, we want to store the account -// ID. -// If the ID is an account ID, it will be returned as is, without performing any API calls. -// If the ID is an organization ID, it will be refreshed via the provided account ID and returned. -func DetermineMixedOrganizationConstraintIDToStore( - ctx context.Context, - client *aiven.Client, - stateID string, - accountID string, -) (string, error) { - if len(accountID) == 0 { - return "", nil - } - - if !isStringAnOrganizationID(stateID) { - return accountID, nil - } +// organizationGetter helper type to shrinks the avngen.Client interface size. +type organizationGetter interface { + OrganizationGet(ctx context.Context, id string) (*organization.OrganizationGetOut, error) +} - r, err := client.Accounts.Get(ctx, accountID) - if err != nil { - return "", err - } +// ConvertOrganizationToAccountID transforms provided ID to an account ID via API call if it is an organization ID. +func ConvertOrganizationToAccountID(ctx context.Context, id string, client organizationGetter) (string, error) { + if IsOrganizationID(id) { + resp, err := client.OrganizationGet(ctx, id) + if err != nil { + return "", err + } - if len(r.Account.OrganizationId) == 0 { - return accountID, nil + return resp.AccountId, nil } - return r.Account.OrganizationId, nil + return id, nil } // StringToDiagWarning is a function that converts a string to a diag warning. diff --git a/internal/schemautil/helpers_test.go b/internal/schemautil/helpers_test.go new file mode 100644 index 000000000..d24c23920 --- /dev/null +++ b/internal/schemautil/helpers_test.go @@ -0,0 +1,85 @@ +package schemautil + +import ( + "context" + "testing" + + "github.com/aiven/go-client-codegen/handler/organization" + "github.com/stretchr/testify/assert" +) + +type mockAvngenClient struct { + get func(ctx context.Context, id string) (*organization.OrganizationGetOut, error) +} + +func (c *mockAvngenClient) OrganizationGet(ctx context.Context, id string) (*organization.OrganizationGetOut, error) { + return c.get(ctx, id) +} + +func TestDetermineMixedOrganizationConstraintIDToStore(t *testing.T) { + t.Parallel() + + var ( + ctx = context.Background() + ) + + type testCase struct { + name string + input string + want string + wantErr bool + } + + tests := []testCase{ + { + name: "provided Organization ID", + input: "org-123", + want: "acc-123", + wantErr: false, + }, + { + name: "provided Account ID", + input: "acc-123", + want: "acc-123", + wantErr: false, + }, + { + name: "provided an empty ID", + input: "", + want: "", + wantErr: false, + }, + { + name: "error when fetching organization", + input: "org-123", + want: "", + wantErr: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + client := new(mockAvngenClient) + client.get = func(_ context.Context, _ string) (*organization.OrganizationGetOut, error) { + if tt.wantErr { + return nil, assert.AnError + } + + return &organization.OrganizationGetOut{AccountId: tt.want}, nil + } + + got, err := ConvertOrganizationToAccountID(ctx, tt.input, client) + if tt.wantErr { + assert.Error(t, err) + + return + } + + assert.NoError(t, err) + assert.Equalf(t, tt.want, got, "expected %s, got %s", tt.want, got) + }) + } + +} diff --git a/internal/sdkprovider/service/organization/organizational_unit.go b/internal/sdkprovider/service/organization/organizational_unit.go index 025c857cc..5e55015dd 100644 --- a/internal/sdkprovider/service/organization/organizational_unit.go +++ b/internal/sdkprovider/service/organization/organizational_unit.go @@ -2,11 +2,13 @@ package organization import ( "context" + "fmt" - "github.com/aiven/aiven-go-client/v2" - "github.com/hashicorp/terraform-plugin-sdk/v2/diag" + avngen "github.com/aiven/go-client-codegen" + "github.com/aiven/go-client-codegen/handler/account" "github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema" + "github.com/aiven/terraform-provider-aiven/internal/common" "github.com/aiven/terraform-provider-aiven/internal/schemautil" ) @@ -41,10 +43,10 @@ var aivenOrganizationalUnitSchema = map[string]*schema.Schema{ func ResourceOrganizationalUnit() *schema.Resource { return &schema.Resource{ Description: "Creates and manages an [organizational unit](https://aiven.io/docs/platform/concepts/orgs-units-projects) in an Aiven organization.", - CreateContext: resourceOrganizationalUnitCreate, - ReadContext: resourceOrganizationalUnitRead, - UpdateContext: resourceOrganizationalUnitUpdate, - DeleteContext: resourceOrganizationalUnitDelete, + CreateContext: common.WithGenClient(resourceOrganizationalUnitCreate), + ReadContext: common.WithGenClient(resourceOrganizationalUnitRead), + UpdateContext: common.WithGenClient(resourceOrganizationalUnitUpdate), + DeleteContext: common.WithGenClient(resourceOrganizationalUnitDelete), Importer: &schema.ResourceImporter{ StateContext: schema.ImportStatePassthroughContext, }, @@ -54,91 +56,114 @@ func ResourceOrganizationalUnit() *schema.Resource { } } -func resourceOrganizationalUnitCreate(ctx context.Context, d *schema.ResourceData, m interface{}) diag.Diagnostics { - client := m.(*aiven.Client) - name := d.Get("name").(string) +func resourceOrganizationalUnitCreate(ctx context.Context, d *schema.ResourceData, client avngen.Client) error { + var ( + name = d.Get("name").(string) + parentID = d.Get("parent_id").(string) + ) - parentID, err := schemautil.NormalizeOrganizationID(ctx, client, d.Get("parent_id").(string)) + accID, err := schemautil.ConvertOrganizationToAccountID(ctx, parentID, client) if err != nil { - return diag.FromErr(err) + return err } - r, err := client.Accounts.Create( - ctx, - aiven.Account{ - Name: name, - ParentAccountId: parentID, - }, - ) + resp, err := client.AccountCreate(ctx, &account.AccountCreateIn{ + AccountName: name, + ParentAccountId: &accID, + PrimaryBillingGroupId: nil, + }) if err != nil { - return diag.FromErr(err) + return err } - d.SetId(r.Account.Id) + d.SetId(resp.AccountId) - return resourceOrganizationalUnitRead(ctx, d, m) + return resourceOrganizationalUnitRead(ctx, d, client) } -func resourceOrganizationalUnitRead(ctx context.Context, d *schema.ResourceData, m interface{}) diag.Diagnostics { - client := m.(*aiven.Client) - - r, err := client.Accounts.Get(ctx, d.Id()) +func resourceOrganizationalUnitRead(ctx context.Context, d *schema.ResourceData, client avngen.Client) error { + resp, err := client.AccountGet(ctx, d.Id()) if err != nil { - return diag.FromErr(schemautil.ResourceReadHandleNotFound(err, d)) + return schemautil.ResourceReadHandleNotFound(err, d) } - if stateID, _ := d.GetOk("parent_id"); true { - idToSet, err := schemautil.DetermineMixedOrganizationConstraintIDToStore( + // the ParentAccountId is required for the resource to be valid and this case should never happen, + // but we still need to check for it due to the definition in the avngen response schema + if resp.ParentAccountId == nil { + return fmt.Errorf("parent_id is not set for organizational unit: %q", d.Id()) + } + + if stateID, ok := d.GetOk("parent_id"); ok { + idToSet, err := determineMixedOrganizationConstraintIDToStore( ctx, client, stateID.(string), - r.Account.ParentAccountId, + *resp.ParentAccountId, ) if err != nil { - return diag.FromErr(err) + return err } - if err := d.Set("parent_id", idToSet); err != nil { - return diag.FromErr(err) + if err = d.Set("parent_id", idToSet); err != nil { + return err } } - if err := d.Set("name", r.Account.Name); err != nil { - return diag.FromErr(err) + if err = schemautil.ResourceDataSet( + aivenOrganizationalUnitSchema, + d, + resp, + ); err != nil { + return err + } + + return nil +} + +func determineMixedOrganizationConstraintIDToStore( + ctx context.Context, + client avngen.Client, + stateID string, + accountID string, +) (string, error) { + if len(accountID) == 0 { + return "", nil } - if err := d.Set("tenant_id", r.Account.TenantId); err != nil { - return diag.FromErr(err) + + if !schemautil.IsOrganizationID(stateID) { + return accountID, nil } - if err := d.Set("create_time", r.Account.CreateTime.String()); err != nil { - return diag.FromErr(err) + + r, err := client.AccountGet(ctx, accountID) + if err != nil { + return "", err } - if err := d.Set("update_time", r.Account.UpdateTime.String()); err != nil { - return diag.FromErr(err) + + if len(r.OrganizationId) == 0 { + return accountID, nil } - return nil + return r.OrganizationId, nil } -func resourceOrganizationalUnitUpdate(ctx context.Context, d *schema.ResourceData, m interface{}) diag.Diagnostics { - client := m.(*aiven.Client) +func resourceOrganizationalUnitUpdate(ctx context.Context, d *schema.ResourceData, client avngen.Client) error { + var name = d.Get("name").(string) - r, err := client.Accounts.Update(ctx, d.Id(), aiven.Account{ - Name: d.Get("name").(string), + resp, err := client.AccountUpdate(ctx, d.Id(), &account.AccountUpdateIn{ + AccountName: &name, }) if err != nil { - return diag.FromErr(err) + return err } - d.SetId(r.Account.Id) + d.SetId(resp.AccountId) - return resourceOrganizationalUnitRead(ctx, d, m) + return resourceOrganizationalUnitRead(ctx, d, client) } -func resourceOrganizationalUnitDelete(ctx context.Context, d *schema.ResourceData, m interface{}) diag.Diagnostics { - client := m.(*aiven.Client) - - if err := client.Accounts.Delete(ctx, d.Id()); err != nil && !aiven.IsNotFound(err) { - return diag.FromErr(err) +func resourceOrganizationalUnitDelete(ctx context.Context, d *schema.ResourceData, client avngen.Client) error { + if err := client.AccountDelete(ctx, d.Id()); common.IsCritical(err) { + return err } return nil diff --git a/internal/sdkprovider/service/organization/organizational_unit_data_source.go b/internal/sdkprovider/service/organization/organizational_unit_data_source.go index 9a6fab2ea..77c1fa132 100644 --- a/internal/sdkprovider/service/organization/organizational_unit_data_source.go +++ b/internal/sdkprovider/service/organization/organizational_unit_data_source.go @@ -2,38 +2,38 @@ package organization import ( "context" + "fmt" - "github.com/aiven/aiven-go-client/v2" - "github.com/hashicorp/terraform-plugin-sdk/v2/diag" + avngen "github.com/aiven/go-client-codegen" "github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema" + "github.com/aiven/terraform-provider-aiven/internal/common" "github.com/aiven/terraform-provider-aiven/internal/schemautil" ) func DatasourceOrganizationalUnit() *schema.Resource { return &schema.Resource{ - ReadContext: datasourceOrganizationalUnitRead, + ReadContext: common.WithGenClient(datasourceOrganizationalUnitRead), Description: "Gets information about an organizational unit.", Schema: schemautil.ResourceSchemaAsDatasourceSchema(aivenOrganizationalUnitSchema, "name"), } } -func datasourceOrganizationalUnitRead(ctx context.Context, d *schema.ResourceData, m interface{}) diag.Diagnostics { - client := m.(*aiven.Client) - +func datasourceOrganizationalUnitRead(ctx context.Context, d *schema.ResourceData, client avngen.Client) error { name := d.Get("name").(string) - r, err := client.Accounts.List(ctx) + resp, err := client.AccountList(ctx) if err != nil { - return diag.FromErr(err) + return err } - for _, ac := range r.Accounts { - if ac.Name == name { - d.SetId(ac.Id) - return resourceOrganizationalUnitRead(ctx, d, m) + for _, ac := range resp { + if ac.AccountName == name { + d.SetId(ac.AccountId) + + return resourceOrganizationalUnitRead(ctx, d, client) } } - return diag.Errorf("organizational unit %s not found", name) + return fmt.Errorf("organizational unit %q not found", name) } diff --git a/internal/sdkprovider/service/organization/organizational_unit_test.go b/internal/sdkprovider/service/organization/organizational_unit_test.go new file mode 100644 index 000000000..4cf6a0003 --- /dev/null +++ b/internal/sdkprovider/service/organization/organizational_unit_test.go @@ -0,0 +1,94 @@ +package organization_test + +import ( + "context" + "fmt" + "testing" + + "github.com/hashicorp/terraform-plugin-testing/helper/acctest" + "github.com/hashicorp/terraform-plugin-testing/helper/resource" + "github.com/hashicorp/terraform-plugin-testing/terraform" + + acc "github.com/aiven/terraform-provider-aiven/internal/acctest" +) + +const orgUnitResource = "aiven_organizational_unit" + +func TestAccAivenOrganizationalUnit(t *testing.T) { + var ( + resourceName = fmt.Sprintf(orgUnitResource + ".bar") + rName = acctest.RandStringFromCharSet(10, acctest.CharSetAlphaNum) + ) + + resource.ParallelTest(t, resource.TestCase{ + PreCheck: func() { acc.TestAccPreCheck(t) }, + ProtoV6ProviderFactories: acc.TestProtoV6ProviderFactories, + CheckDestroy: testAccCheckAivenOrganizationalUnitDestroy, + Steps: []resource.TestStep{ + { + Config: testAccOrganizationalUnitResource(rName), + Check: resource.ComposeTestCheckFunc( + resource.TestCheckResourceAttr(resourceName, "name", fmt.Sprintf("test-acc-unit-%s", rName)), + resource.TestCheckResourceAttrSet(resourceName, "parent_id"), + resource.TestCheckResourceAttrSet(resourceName, "tenant_id"), + resource.TestCheckResourceAttrSet(resourceName, "create_time"), + resource.TestCheckResourceAttrSet(resourceName, "update_time"), + ), + }, + { + Config: testAccOrganizationalUnitResource(rName + "_updated"), + Check: resource.ComposeTestCheckFunc( + resource.TestCheckResourceAttr(resourceName, "name", fmt.Sprintf("test-acc-unit-%s", rName+"_updated")), + resource.TestCheckResourceAttrSet(resourceName, "parent_id"), + resource.TestCheckResourceAttrSet(resourceName, "tenant_id"), + resource.TestCheckResourceAttrSet(resourceName, "create_time"), + resource.TestCheckResourceAttrSet(resourceName, "update_time"), + ), + }, + }, + }) +} + +func testAccOrganizationalUnitResource(name string) string { + return fmt.Sprintf(` +resource "aiven_organization" "foo" { + name = "test-acc-orgu-%s" +} + +resource "aiven_organizational_unit" "bar" { + name = "test-acc-unit-%s" + parent_id = aiven_organization.foo.id +} +`, name, name) +} + +func testAccCheckAivenOrganizationalUnitDestroy(s *terraform.State) error { + var ( + c, err = acc.GetTestGenAivenClient() + ctx = context.Background() + ) + + if err != nil { + return fmt.Errorf("failed to instantiate GenAiven client: %w", err) + } + + // loop through the resources in state, verifying that organizational unit account is destroyed + for _, rs := range s.RootModule().Resources { + if rs.Type != orgUnitResource { + continue + } + + resp, err := c.AccountList(ctx) + if err != nil { + return fmt.Errorf("error listing accounts: %w", err) + } + + for _, account := range resp { + if account.AccountId == rs.Primary.ID { + return fmt.Errorf("organizational unit (%q) still exists", rs.Primary.ID) + } + } + } + + return nil +} diff --git a/internal/sdkprovider/service/project/billing_group.go b/internal/sdkprovider/service/project/billing_group.go index f4c70e6e2..28e41b9cd 100644 --- a/internal/sdkprovider/service/project/billing_group.go +++ b/internal/sdkprovider/service/project/billing_group.go @@ -180,14 +180,14 @@ func resourceBillingGroupRead(ctx context.Context, d *schema.ResourceData, m int return diag.FromErr(schemautil.ResourceReadHandleNotFound(err, d)) } - if stateID, _ := d.GetOk("parent_id"); true { + if stateID, ok := d.GetOk("parent_id"); ok { var accountID string if bg.AccountId != nil { accountID = *bg.AccountId } - idToSet, err := schemautil.DetermineMixedOrganizationConstraintIDToStore( + idToSet, err := determineMixedOrganizationConstraintIDToStore( ctx, client, stateID.(string), diff --git a/internal/sdkprovider/service/project/project.go b/internal/sdkprovider/service/project/project.go index 411753c00..833a3340b 100644 --- a/internal/sdkprovider/service/project/project.go +++ b/internal/sdkprovider/service/project/project.go @@ -432,7 +432,7 @@ func setProjectTerraformProperties( project *aiven.Project, ) diag.Diagnostics { if stateID := d.Get("parent_id"); stateID != "" { - idToSet, err := schemautil.DetermineMixedOrganizationConstraintIDToStore( + idToSet, err := determineMixedOrganizationConstraintIDToStore( ctx, client, stateID.(string), diff --git a/internal/sdkprovider/service/project/util.go b/internal/sdkprovider/service/project/util.go index 708fa9ed8..c982b8754 100644 --- a/internal/sdkprovider/service/project/util.go +++ b/internal/sdkprovider/service/project/util.go @@ -34,3 +34,36 @@ func accountIDPointer(ctx context.Context, client *aiven.Client, d *schema.Resou return accountID, nil } + +// determineMixedOrganizationConstraintIDToStore is a helper function that returns the ID to store in the state. +// We have several fields that can be either an organization ID or an account ID. +// We want to store the one that was already in the state, if it was already there. +// If it was not, we want to prioritize the organization ID, but if it is not available, we want to store the account +// ID. +// If the ID is an account ID, it will be returned as is, without performing any API calls. +// If the ID is an organization ID, it will be refreshed via the provided account ID and returned. +func determineMixedOrganizationConstraintIDToStore( + ctx context.Context, + client *aiven.Client, + stateID string, + accountID string, +) (string, error) { + if accountID == "" { + return "", nil + } + + if !schemautil.IsOrganizationID(stateID) { + return accountID, nil + } + + r, err := client.Accounts.Get(ctx, accountID) + if err != nil { + return "", err + } + + if r.Account.OrganizationId != "" { + return r.Account.OrganizationId, nil + } + + return accountID, nil +}