Fix: env0_aws_credentials - apply after import will delete+create #371

TomerHeber · 2022-05-10T19:22:11Z

I'm keeping open, to adding support for other credential types.

Solution

Added import.
Removed a schema requirements and instead enforce during read.
Added acceptance tests.
Added a "resource" tag.
Added import examples.
Fixed some other issues I noticed.

TomerHeber · 2022-05-10T19:24:35Z

client/cloud_credentials.go

@@ -37,7 +33,7 @@ type AwsCredentialsCreatePayload struct {
 }

 type AwsCredentialsValuePayload struct {
-	RoleArn         string `json:"roleArn"`
+	RoleArn         string `json:"roleArn" resource:"arn"`


new tag type for serialization. See usage below.
(Basically in terraform the resource is "arn" and not "role_arn" - so needed to resolve this).

TomerHeber · 2022-05-10T19:24:54Z

client/cloud_credentials.go

@@ -89,7 +85,7 @@ func (client *ApiClient) CloudCredentials(id string) (Credentials, error) {
 		}
 	}

-	return Credentials{}, fmt.Errorf("CloudCredentials: [%s] not found ", id)
+	return Credentials{}, &NotFoundError{}


Used to handle drift later.

TomerHeber · 2022-05-10T19:25:35Z

env0/credentials.go

+	"github.com/google/uuid"
+)
+
+func getCredentialsByName(name string, prefix string, meta interface{}) (client.Credentials, error) {


Common helper functions for credentials (can be used by GCP and Azure in the future). (In case import is added to them as well).

TomerHeber · 2022-05-10T19:25:54Z

env0/data_notification.go

@@ -61,7 +61,7 @@ func dataNotificationRead(ctx context.Context, d *schema.ResourceData, meta inte
 	}

 	if err := writeResourceData(notification, d); err != nil {
-		diag.Errorf("schema resource data serialization failed: %v", err)
+		return diag.Errorf("schema resource data serialization failed: %v", err)


there are a few places with the same bug...

TomerHeber · 2022-05-10T19:27:17Z

env0/resource_aws_credentials.go

@@ -27,7 +30,6 @@ func resourceAwsCredentials() *schema.Resource {
 				Optional:      true,
 				ForceNew:      true,
 				ConflictsWith: []string{"access_key_id"},
-				ExactlyOneOf:  []string{"access_key_id"},


The "ExactlyOneOf" cannot be enforced in the schema level. (Will fail on import).
(Enforced in the code level).

TomerHeber · 2022-05-10T19:27:55Z

env0/resource_aws_credentials.go

@@ -62,29 +63,32 @@ func resourceAwsCredentials() *schema.Resource {
 }

 func resourceAwsCredentialsCreate(ctx context.Context, d *schema.ResourceData, meta interface{}) diag.Diagnostics {
+	_, accessKeyExist := d.GetOk("access_key_id")


Enforcing the exactlyOnce in the code level only for creation (not for read/import).

TomerHeber · 2022-05-10T19:28:45Z

env0/resource_aws_credentials.go

-	}
-	if externalId, ok := d.GetOk("external_id"); ok {
-		value.ExternalId = externalId.(string)
+	if err := readResourceData(&value, d); err != nil {


some small refactoring (less lines of code).

TomerHeber · 2022-05-10T19:30:48Z

env0/utils.go

 		fieldNameSC := toSnakeCase(fieldName)
+		if resFieldName, ok := val.Type().Field(i).Tag.Lookup("resource"); ok {


This is for the new tag I created (resource). Ping me if this is still unclear.

Very cool 👏
Just wondering together with you whether resourcee is a decent tag name or we can come up with a better name. Maybe something that indicates that this is related to the TF schema would do better.

Also - WDYT about adding some note regarding it in DEVELOPMENT.md?

yes and yes.
How about tfschema or tfresource ? (I'm bad with names).

Both are better. If you want me to choose then I'd go with tfschema 😅

we have a winner (:

TomerHeber · 2022-05-10T19:31:21Z

env0/utils_test.go

@@ -91,6 +91,24 @@ func TestReadResourceDataNotification(t *testing.T) {
 	assert.Equal(t, expectedPayload, payload)
 }

+func TestReadResourceDataWithTag(t *testing.T) {


test for the new resource tag (hopefully this test helps clarify it).

TomerHeber · 2022-05-10T19:36:39Z

env0/resource_aws_credentials_test.go

@@ -108,7 +116,7 @@ func TestUnitAwsCredentialsResource(t *testing.T) {
 		Steps: []resource.TestStep{
 			{
 				Config:      resourceConfigCreate(resourceType, resourceName, mutuallyExclusiveErrorResource),
-				ExpectError: regexp.MustCompile("only one of `access_key_id,arn` can be specified"),
+				ExpectError: regexp.MustCompile(`"external_id": conflicts with access_key_id`),


There are multiple errors., the following just come before.

yaronya

Nice work!
Left some few minor comments

yaronya · 2022-05-12T19:54:14Z

env0/resource_aws_credentials.go

+	d.Set("name", credentials.Name)
+	d.SetId(credentials.Id)


Can we use the seralization logic here instead?

yeah sure. In general, I use it when there a multiple fields.
Either way is fine by me. I'll fix it.

I think always using it makes a lot of sense since we wouldn't need to change anything when we'll add a new field 💪

env0/resource_aws_credentials.go

yaronya · 2022-05-12T19:55:20Z

env0/utils.go

 		fieldNameSC := toSnakeCase(fieldName)
+		if resFieldName, ok := val.Type().Field(i).Tag.Lookup("resource"); ok {


Very cool 👏
Just wondering together with you whether resourcee is a decent tag name or we can come up with a better name. Maybe something that indicates that this is related to the TF schema would do better.

Also - WDYT about adding some note regarding it in DEVELOPMENT.md?

yaronya · 2022-05-12T21:09:54Z

env0/resource_aws_credentials.go

@@ -14,6 +15,8 @@ func resourceAwsCredentials() *schema.Resource {
 		ReadContext:   resourceAwsCredentialsRead,
 		DeleteContext: resourceAwsCredentialsDelete,

+		Importer: &schema.ResourceImporter{StateContext: resourceAwsCredentialsImport},


@TomerHeber if we're already at it, can we perhaps also support import for all other credentials type? I'd do it in a separate PR tho

keeping the issue open.

TomerHeber added the pending final review label May 10, 2022

TomerHeber self-assigned this May 10, 2022

github-actions bot added fix api-client examples provider labels May 10, 2022

TomerHeber commented May 10, 2022

View reviewed changes

TomerHeber requested a review from yaronya May 10, 2022 19:32

TomerHeber commented May 10, 2022

View reviewed changes

yaronya reviewed May 12, 2022

View reviewed changes

yaronya added reviewed - awaiting author and removed pending final review labels May 12, 2022

yaronya reviewed May 12, 2022

View reviewed changes

TomerHeber added 2 commits May 13, 2022 08:07

Fix: env0_aws_credentials - apply after import will delete+create

d3cdc90

improvements based on CR comments

7d06da7

TomerHeber force-pushed the fix-apply-will-delete-create-#99 branch from ebd19fc to 7d06da7 Compare May 13, 2022 13:20

TomerHeber added pending final review and removed reviewed - awaiting author labels May 13, 2022

TomerHeber requested a review from yaronya May 13, 2022 13:21

yaronya approved these changes May 13, 2022

View reviewed changes

github-actions bot added ready to merge PR approved - can be merged once the PR owner is ready and removed pending final review labels May 13, 2022

TomerHeber merged commit f488f56 into main May 13, 2022

TomerHeber deleted the fix-apply-will-delete-create-#99 branch May 13, 2022 15:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix: env0_aws_credentials - apply after import will delete+create #371

Fix: env0_aws_credentials - apply after import will delete+create #371

TomerHeber commented May 10, 2022 •

edited

Loading

TomerHeber May 10, 2022

TomerHeber May 10, 2022

TomerHeber May 10, 2022

TomerHeber May 10, 2022

TomerHeber May 10, 2022

TomerHeber May 10, 2022 •

edited

Loading

TomerHeber May 10, 2022

TomerHeber May 10, 2022

TomerHeber May 10, 2022

yaronya May 12, 2022

TomerHeber May 12, 2022

yaronya May 12, 2022 •

edited

Loading

TomerHeber May 12, 2022

TomerHeber May 13, 2022

TomerHeber May 10, 2022

TomerHeber May 10, 2022

yaronya left a comment

yaronya May 12, 2022

TomerHeber May 12, 2022

yaronya May 12, 2022

TomerHeber May 13, 2022

yaronya May 12, 2022

yaronya May 12, 2022

TomerHeber May 13, 2022

		fieldNameSC := toSnakeCase(fieldName)
		if resFieldName, ok := val.Type().Field(i).Tag.Lookup("resource"); ok {

Fix: env0_aws_credentials - apply after import will delete+create #371

Fix: env0_aws_credentials - apply after import will delete+create #371

Conversation

TomerHeber commented May 10, 2022 • edited Loading

Solution

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TomerHeber May 10, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yaronya May 12, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yaronya left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TomerHeber commented May 10, 2022 •

edited

Loading

TomerHeber May 10, 2022 •

edited

Loading

yaronya May 12, 2022 •

edited

Loading