Feat: add wait_for_destroy option when destroying and environment #946

TomerHeber · 2024-09-01T16:17:33Z

Issue & Steps to Reproduce / Feature Request

resolves #686

Solution

Added the option in the schema.
When an environment is 'destroyed', added the 'wait' code if the option is enabled.
Added acceptance tests.
Some misc changes to enable (in the future) a more modern linter checkers.

TomerHeber · 2024-09-01T16:19:08Z

env0/data_agent_values_test.go

@@ -63,5 +63,4 @@ func TestAgentValues(t *testing.T) {
 			},
 		)
 	})
-


ignore all these changes... there are thousands of changes required to successfully add a new linter, so I slowly fix these with each PR I make.

TomerHeber · 2024-09-01T16:19:35Z

env0/resource_environment.go

@@ -368,6 +370,12 @@ func resourceEnvironment() *schema.Resource {
 					Description: "variable set id",
 				},
 			},
+			"wait_for_destroy": {
+				Type:        schema.TypeBool,
+				Description: "during destroy, waits for the environment status to be 'INACTIVE'. Times out after 30 minutes.",


I hope the description is clear enough.

TomerHeber · 2024-09-01T16:20:15Z

env0/resource_environment.go

 	if err != nil {
 		if frerr, ok := err.(*http.FailedResponseError); ok && frerr.BadRequest() {
 			tflog.Warn(ctx, "Could not delete environment. Already deleted?", map[string]interface{}{"id": d.Id(), "error": frerr.Error()})
 			return nil
 		}
 		return diag.Errorf("could not delete environment: %v", err)
 	}
+
+	if environment.Status != "INACTIVE" && d.Get("wait_for_destroy").(bool) {


the wait will only be applied if 'wait_for_destroy' is enabled.

TomerHeber · 2024-09-01T16:21:18Z

env0/resource_environment.go

+	timer := time.NewTimer(timeout)       // When invoked - time out
+	results := make(chan error)
+
+	go func() {


this is some golang specific stuff (channels) not very common in other languages.
https://go.dev/tour/concurrency/2

env0/resource_environment.go

TomerHeber · 2024-09-01T16:23:59Z

env0/resource_environment.go

+
+				environment = &latestEnvironment
+
+				if environment.Status == "INACTIVE" {


destroy successful...

yaronya · 2024-09-02T12:48:09Z

env0/resource_environment.go

+	timeout := time.Minute * 30
+	if os.Getenv("TF_ACC") == "1" { // For acceptance tests.
+		waitInteval = time.Second
+		timeout = time.Second * 10


What does it mean that it's 10 seconds?
Would it fail if environment couldn't destroy in 10 seconds?

this is for unit tests...

If it's for UTs - does it mean that test will hang for 10s?

What about harness tests?

Yes... but unit tests run in parallel so it doesn't really cause much problems. (max timeout it 10 seconds)
Harness tests - at least 10 seconds (time between intervals). But harness also run in parallel, so no issues IMHO.

Having 10 seconds timeout for harness tests is too low. Usually deployments take about 5-10 seconds only to start (i.e schedule a worker that handles the deployment), so I suggest we keep it as 3 minutes.

the 10 seconds timeout is for acceptance tests, not harness tests.

Just to clarify - for harness tests it'd keep the original behavior, correct?

yaronya · 2024-09-02T12:49:54Z

env0/resource_environment.go

+					return
+				}
+
+				environment = &latestEnvironment


Why this extra assignment needed?

avoids a few lines of code for the first iteration... but I can check if it can be avoided.

yaronya · 2024-09-02T12:55:20Z

env0/resource_environment.go

@@ -929,17 +937,69 @@ func resourceEnvironmentDelete(ctx context.Context, d *schema.ResourceData, meta
 		return diag.Errorf(`must enable "force_destroy" safeguard in order to destroy`)
 	}

-	_, err := apiClient.EnvironmentDestroy(d.Id())
+	environment, err := apiClient.EnvironmentDestroy(d.Id())


The destroy API returns the deployment log of the new destroy deployment, and not the environment

hmmm if that's the case - sounds like a long standing bug...
I'll fix it....

env0/resource_environment.go

env0/resource_environment_scheduling_test.go

TomerHeber · 2024-09-03T13:25:32Z

client/api_client.go

@@ -66,7 +66,7 @@ type ApiClientInterface interface {
 	Environment(id string) (Environment, error)
 	EnvironmentCreate(payload EnvironmentCreate) (Environment, error)
 	EnvironmentCreateWithoutTemplate(payload EnvironmentCreateWithoutTemplate) (Environment, error)
-	EnvironmentDestroy(id string) (Environment, error)
+	EnvironmentDestroy(id string) (*EnvironmentDestroyResponse, error)


This was an old 'bug'.

TomerHeber · 2024-09-03T13:26:01Z

client/environment.go

@@ -91,6 +93,7 @@ type DeploymentLog struct {
 	Output              json.RawMessage `json:"output,omitempty"`
 	Error               json.RawMessage `json:"error,omitempty"`
 	Type                string          `json:"type"`
+	Status              string          `json:"status"`


added Status to the DeploymentLog sruct.

TomerHeber · 2024-09-03T13:26:33Z

client/environment.go

@@ -283,13 +299,13 @@ func (client *ApiClient) EnvironmentCreateWithoutTemplate(payload EnvironmentCre
 	return result, nil
 }

-func (client *ApiClient) EnvironmentDestroy(id string) (Environment, error) {
-	var result Environment
+func (client *ApiClient) EnvironmentDestroy(id string) (*EnvironmentDestroyResponse, error) {


old bug fixed...

TomerHeber · 2024-09-03T13:28:01Z

env0/resource_environment.go

-					results <- nil
-					return
-				}
+			if slices.Contains([]string{"WAITING_FOR_USER", "TIMEOUT", "FAILURE", "CANCELLED", "INTERNAL_FAILURE", "ABORTING", "ABORTED", "SKIPPED", "NEVER_DEPLOYED"}, deployment.Status) {


(fast) fail on any of these status.

TomerHeber · 2024-09-03T13:28:13Z

env0/resource_environment.go

-				if environment.Status == "DESTROY_IN_PROGRESS" {
-					continue
-				}
+			if deployment.Status == "SUCCESS" {


happy path. After waiting...

TomerHeber · 2024-09-03T13:28:44Z

env0/resource_environment.go

 				return
+			case <-ticker.C:
+				continue


keep waiting...

TomerHeber · 2024-09-03T13:29:15Z

tests/integration/012_environment/main.tf

@@ -49,6 +49,7 @@ resource "env0_environment" "auto_glob_envrironment" {
  approve_plan_automatically       = true
  deploy_on_push                   = true
  force_destroy                    = true
+  wait_for_destroy                 = true


added the flag to one of the harness tests...

yaronya · 2024-09-24T12:35:48Z

@TomerHeber can you please resolve all conflicts before I review?

TomerHeber · 2024-09-24T12:56:05Z

@TomerHeber can you please resolve all conflicts before I review?

@yaronya - done!

yaronya · 2024-09-04T07:28:37Z

client/environment.go

@@ -256,6 +263,15 @@ func (client *ApiClient) Environment(id string) (Environment, error) {
 	return result, nil
 }

+func (client *ApiClient) EnvironmentDeployment(id string) (*DeploymentLog, error) {


I suggest to call it EnvironmentDeploymentLog so it'd be clearer to the dev that it's different from EnvironmentDeploy

client/environment.go

yaronya · 2024-10-08T11:20:25Z

env0/resource_environment.go

+				return
+			}
+
+			if slices.Contains([]string{"WAITING_FOR_USER", "TIMEOUT", "FAILURE", "CANCELLED", "INTERNAL_FAILURE", "ABORTING", "ABORTED", "SKIPPED", "NEVER_DEPLOYED"}, deployment.Status) {


I suggest to not fail on WAITING_FOR_USER but rather keep on waiting, and maybe write something back to the user that the deployment is waiting for an approval

This is actually something to consider for other statuses as well, like QUEUED, so how about we print the current status on every iteration?

Added additional messages, and no longer fast failing with WAITING_FOR_USER

yaronya · 2024-10-08T11:27:01Z

env0/resource_environment.go

+	timeout := time.Minute * 30
+	if os.Getenv("TF_ACC") == "1" { // For acceptance tests.
+		waitInteval = time.Second
+		timeout = time.Second * 10


Having 10 seconds timeout for harness tests is too low. Usually deployments take about 5-10 seconds only to start (i.e schedule a worker that handles the deployment), so I suggest we keep it as 3 minutes.

TomerHeber · 2024-10-08T13:09:21Z

@yaronya - this is the relevant commit 351976c

yaronya · 2024-10-14T15:41:01Z

env0/resource_environment.go

+	timeout := time.Minute * 30
+	if os.Getenv("TF_ACC") == "1" { // For acceptance tests.
+		waitInteval = time.Second
+		timeout = time.Second * 10


Just to clarify - for harness tests it'd keep the original behavior, correct?

TomerHeber added the pending final review label Sep 1, 2024

TomerHeber self-assigned this Sep 1, 2024

github-actions bot added feature provider labels Sep 1, 2024

TomerHeber commented Sep 1, 2024

View reviewed changes

env0/resource_environment.go Outdated Show resolved Hide resolved

TomerHeber requested a review from yaronya September 1, 2024 16:22

TomerHeber commented Sep 1, 2024

View reviewed changes

yaronya reviewed Sep 2, 2024

View reviewed changes

github-actions bot added api-client integration-tests labels Sep 3, 2024

TomerHeber commented Sep 3, 2024

View reviewed changes

env0/resource_environment.go

return

case <-ticker.C:

continue

Copy link

Collaborator Author

TomerHeber Sep 3, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

keep waiting...

TomerHeber commented Sep 3, 2024

View reviewed changes

TomerHeber requested a review from yaronya September 3, 2024 13:29

TomerHeber force-pushed the feat-wait_for_destory-#686 branch from 8988667 to e12d69b Compare September 24, 2024 12:49

TomerHeber added 4 commits October 1, 2024 20:16

Feat: add wait_for_destroy option when destroying and environment

cc32ff5

changes based on PR comments

a166763

minor change

9619502

added disclaimer

bc20d5b

TomerHeber force-pushed the feat-wait_for_destory-#686 branch from e12d69b to bc20d5b Compare October 2, 2024 01:22

yaronya reviewed Oct 8, 2024

View reviewed changes

TomerHeber and others added 2 commits October 8, 2024 07:29

Merge branch 'main' into feat-wait_for_destory-#686

dbfe817

made changes based on PR feedback

351976c

TomerHeber requested a review from yaronya October 8, 2024 13:08

yaronya approved these changes Oct 14, 2024

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 Oct 14, 2024

TomerHeber merged commit af33272 into main Oct 14, 2024
7 checks passed

TomerHeber deleted the feat-wait_for_destory-#686 branch October 14, 2024 15:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feat: add wait_for_destroy option when destroying and environment #946

Feat: add wait_for_destroy option when destroying and environment #946

TomerHeber commented Sep 1, 2024

TomerHeber Sep 1, 2024 •

edited

Loading

TomerHeber Sep 1, 2024

TomerHeber Sep 1, 2024

TomerHeber Sep 1, 2024

TomerHeber Sep 1, 2024

yaronya Sep 2, 2024

TomerHeber Sep 2, 2024

TomerHeber Sep 2, 2024

yaronya Sep 2, 2024

TomerHeber Sep 24, 2024

yaronya Oct 8, 2024

TomerHeber Oct 8, 2024 •

edited

Loading

yaronya Oct 14, 2024

TomerHeber Oct 14, 2024

yaronya Sep 2, 2024

TomerHeber Sep 2, 2024 •

edited

Loading

yaronya Sep 2, 2024

TomerHeber Sep 2, 2024

TomerHeber Sep 3, 2024

TomerHeber Sep 3, 2024

TomerHeber Sep 3, 2024

TomerHeber Sep 3, 2024 •

edited

Loading

TomerHeber Sep 3, 2024

TomerHeber Sep 3, 2024

TomerHeber Sep 3, 2024

yaronya commented Sep 24, 2024

TomerHeber commented Sep 24, 2024

yaronya Sep 4, 2024

yaronya Oct 8, 2024

TomerHeber Oct 8, 2024

yaronya Oct 8, 2024

TomerHeber commented Oct 8, 2024

yaronya Oct 14, 2024

@@ @@ -63,5 +63,4 @@ func TestAgentValues(t *testing.T) { @@
               			},
               		)
               	})


		environment = &latestEnvironment

		if environment.Status == "INACTIVE" {

Feat: add wait_for_destroy option when destroying and environment #946

Feat: add wait_for_destroy option when destroying and environment #946

Conversation

TomerHeber commented Sep 1, 2024

Issue & Steps to Reproduce / Feature Request

Solution

TomerHeber Sep 1, 2024 • 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

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 Oct 8, 2024 • 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

TomerHeber Sep 2, 2024 • 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

TomerHeber Sep 3, 2024 • 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

yaronya commented Sep 24, 2024

TomerHeber commented Sep 24, 2024

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 Oct 8, 2024

Choose a reason for hiding this comment

TomerHeber Sep 1, 2024 •

edited

Loading

TomerHeber Oct 8, 2024 •

edited

Loading

TomerHeber Sep 2, 2024 •

edited

Loading

TomerHeber Sep 3, 2024 •

edited

Loading