Feat: support new apply-all environment property #893

TomerHeber · 2024-07-10T14:55:31Z

Issue & Steps to Reproduce / Feature Request

resolves #890

Solution

Added the new field in the APIs for project policy and environment.
Updated the schemas for project policy and environment.
Updated the acceptance tests.
Updated the integration tests.

TomerHeber · 2024-07-10T14:56:11Z

client/environment.go

@@ -122,6 +122,7 @@ type Environment struct {
 	LatestDeploymentLog         DeploymentLog `json:"latestDeploymentLog"`
 	TerragruntWorkingDirectory  string        `json:"terragruntWorkingDirectory,omitempty"`
 	VcsCommandsAlias            string        `json:"vcsCommandsAlias"`
+	VcsPrCommentsEnabled        bool          `json:"vcsPrCommentsEnabled" tfschema:"-"`


special handling to avoid drifts.

@TomerHeber can you explain more?
we indeed need that-
if a customer

use the the provider version

do not set vcsPrCommentsEnabled

do set VcsCommandsAlias

then the BE response may return vcsPrCommentsEnabled true in the response.
we should not make a drift for that.

use case - a customer that used the alias in old provider version, then did an upgrade.

yeah - in the schema it will be false.
But in the API it will pass true... because alias is used in the schema.
There's a part where this drift is ignored.

The answer has changed.
the return value will now be ignored if nothing was configured in the schema. Not a perfect solution, but should work well for customers.

env0/resource_environment.go

TomerHeber · 2024-07-10T14:57:45Z

env0/resource_environment.go

@@ -380,6 +386,12 @@ func setEnvironmentSchema(ctx context.Context, d *schema.ResourceData, environme
 		return fmt.Errorf("schema resource data serialization failed: %v", err)
 	}

+	if val := d.Get("vcs_pr_comments_enabled").(bool); !val && environment.VcsPrCommentsEnabled {


this is important. Although the default is 'false' it's possible vcs_pr_comments_enabled is set to 'true' (E.g. alias was used).
To avoid drifts - it's ignored in this case.

TomerHeber · 2024-07-10T14:58:07Z

env0/resource_environment.go

@@ -882,6 +894,11 @@ func getCreatePayload(d *schema.ResourceData, apiClient client.ApiClientInterfac
 		return client.EnvironmentCreate{}, diag.Errorf("schema resource data deserialization failed: %v", err)
 	}

+	if val := d.Get("vcs_commands_alias").(string); len(val) > 0 {
+		// if alias is set - VcsPrCommentsEnabled must be true.
+		payload.VcsPrCommentsEnabled = true


alias is used - pass 'true' in the API call.

We actually have code in the Backend that does that - sets to true if alias is given. Maybe it's not necessary here too.. WDYT?

oh! interesting...

but... I kinda like having this logic here... @razbensimon WDYT?

what it adds to the provider user if this logic exists already in BE?

TomerHeber · 2024-07-10T14:58:13Z

env0/resource_environment.go

@@ -1002,6 +1019,11 @@ func getUpdatePayload(d *schema.ResourceData) (client.EnvironmentUpdate, diag.Di
 		return client.EnvironmentUpdate{}, diag.Errorf("schema resource data deserialization failed: %v", err)
 	}

+	if val := d.Get("vcs_commands_alias").(string); len(val) > 0 {
+		// if alias is set - VcsPrCommentsEnabled must be true.
+		payload.VcsPrCommentsEnabled = true


alias is used - pass 'true' in the API call.

TomerHeber · 2024-07-10T14:59:13Z

env0/resource_project_policy.go

@@ -111,6 +111,12 @@ func resourceProjectPolicy() *schema.Resource {
 				Optional:         true,
 				ValidateDiagFunc: ValidateCronExpression,
 			},
+			"vcs_pr_comments_enabled_default": {
+				Type:        schema.TypeBool,
+				Description: "if 'true' all environments created in this project via the UI will be created with an 'enabled' running VCS PR commands using PR comments. Note: in the provider when creating an environment this is ignored, and must be configured explicitly by setting 'vcs_pr_comments_enabled' to 'true' or setting an alias in 'vcs_commands_alias'.",


See the note.
This is a design decision for the provider.

nice. But what do you mean by design decision for the provider.?
This is the purpose of the project policies, no?

going to make changes based on our discussion.

@TomerHeber Not sure I got this too.
it should be something like this:

Suggested change

Description: "if 'true' all environments created in this project via the UI will be created with an 'enabled' running VCS PR commands using PR comments. Note: in the provider when creating an environment this is ignored, and must be configured explicitly by setting 'vcs_pr_comments_enabled' to 'true' or setting an alias in 'vcs_commands_alias'.",

Description: "if 'true' all new environments created in this project will be created with an 'enabled' VCS PR commands using PR comments.",

for a user to enable this feature, he should either provide
'vcs_pr_comments_enabled' to 'true' or setting an alias in 'vcs_commands_alias', or set both which is explicit.

we can add a warning log when the user uses only vcs_commands_alias, this is just a suggestion.
Anyways the BE takes care of it and forcibly set vcs_pr_comments_enabled if needed (not depending on client UI/provider/API)

let me know if we didnt understand you

Indeed if the user enabled the project policy - but didn't add vcs_pr_comments_enabled on the environment resource IaC. this is a problem.

how we should handle this?

sagydr · 2024-07-10T17:33:07Z

tests/integration/011_policy/main.tf

+  skip_apply_when_plan_is_empty   = true
+  disable_destroy_environments    = true
+  skip_redundant_deployments      = true
+  vcs_pr_comments_enabled_default = true


This tests the project policy configuration?

just sanity really.
The better test is the acceptance testing.

sagydr

looks good, approved with minor comments/questions

razbensimon · 2024-07-10T17:44:05Z

client/project_policy.go

@@ -18,6 +18,7 @@ type Policy struct {
 	ForceRemoteBackend          bool    `json:"forceRemoteBackend"`
 	DriftDetectionCron          string  `json:"driftDetectionCron"`
 	DriftDetectionEnabled       bool    `json:"driftDetectionEnabled"`
+	VcsPrCommentsEnabledDefault bool    `json:"vcsPrCommentsEnabledDefault"`


can we put this one bellow ContinuousDeploymentDefault?
this is just my OCD yelling, and want those 3 to be together...

razbensimon · 2024-07-10T17:47:18Z

env0/resource_environment.go

+			"vcs_pr_comments_enabled": {
+				Type:        schema.TypeBool,
+				Description: "set to 'true' to enable running VCS PR commands using PR comments. This can be set to 'true' (enabled) without setting alias in 'vcs_commands_alias'.",


also can add docs link https://docs.env0.com/docs/plan-and-apply-from-pr-comments

Suggested change

"vcs_pr_comments_enabled": {

Type: schema.TypeBool,

Description: "set to 'true' to enable running VCS PR commands using PR comments. This can be set to 'true' (enabled) without setting alias in 'vcs_commands_alias'.",

"vcs_pr_comments_enabled": {

Type: schema.TypeBool,

Description: "set to 'true' to enable running VCS PR plan/apply commands using PR comments. This can be set to 'true' (enabled) without setting alias in 'vcs_commands_alias'.",

Maybe even specifically to the Configuration section:
https://docs.env0.com/docs/plan-and-apply-from-pr-comments#configuration

TomerHeber · 2024-07-13T15:54:13Z

client/environment.go

@@ -144,6 +145,7 @@ type EnvironmentCreate struct {
 	TTL                         *TTL                     `json:"ttl,omitempty" tfschema:"-"`
 	TerragruntWorkingDirectory  string                   `json:"terragruntWorkingDirectory,omitempty"`
 	VcsCommandsAlias            string                   `json:"vcsCommandsAlias"`
+	VcsPrCommentsEnabled        *bool                    `json:"vcsPrCommentsEnabled,omitempty" tfschema:"-"`


bool ptr to support tri-state - true/false/undefined.

TomerHeber · 2024-07-13T15:55:37Z

client/environment.go

@@ -193,6 +195,7 @@ type EnvironmentUpdate struct {
 	AutoDeployByCustomGlob      string `json:"autoDeployByCustomGlob"`
 	TerragruntWorkingDirectory  string `json:"terragruntWorkingDirectory,omitempty"`
 	VcsCommandsAlias            string `json:"vcsCommandsAlias,omitempty"`
+	VcsPrCommentsEnabled        *bool  `json:"vcsPrCommentsEnabled,omitempty" tfschema:"-"`


bool ptr to support tri-state - true/false/undefined. (this is for update)

TomerHeber · 2024-07-13T15:56:21Z

env0/resource_environment.go

+			"vcs_pr_comments_enabled": {
+				Type:        schema.TypeBool,
+				Description: "set to 'true' to enable running VCS PR plan/apply commands using PR comments. This can be set to 'true' (enabled) without setting alias in 'vcs_commands_alias'. Additional details: https://docs.env0.com/docs/plan-and-apply-from-pr-comments#configuration",
+				Optional:    true,


generally it's good practice to set a default value.
But we need to support a tri-state. Setting a default, will enforce it to be only 2 states.

TomerHeber · 2024-07-13T15:57:03Z

env0/resource_environment.go

@@ -380,6 +385,13 @@ func setEnvironmentSchema(ctx context.Context, d *schema.ResourceData, environme
 		return fmt.Errorf("schema resource data serialization failed: %v", err)
 	}

+	//lint:ignore SA1019 reason: https://github.com/hashicorp/terraform-plugin-sdk/issues/817
+	if _, exists := d.GetOkExists("vcs_pr_comments_enabled"); !exists {


if not configured in the schema ignore whatever is returned in the response.
This is not perfect, but will work well for customers.

TomerHeber · 2024-07-13T15:59:33Z

env0/resource_environment.go

+	}
+
+	//lint:ignore SA1019 reason: https://github.com/hashicorp/terraform-plugin-sdk/issues/817
+	if val, exists := d.GetOkExists("vcs_pr_comments_enabled"); exists {


hasChanges won't work in some scenarios. I had to use GetOkExists.
This would mean that some cases (and only if the schema is modified!) it may return 'true' for this when it shouldn't.
This would just mean an extra API call. But everything will work as expected.

TomerHeber · 2024-07-13T16:00:07Z

env0/resource_environment.go

+	if val, exists := d.GetOkExists("vcs_pr_comments_enabled"); exists {
+		// if this field is set to 'false' will return that there is change each time.
+		// this is because the terraform SDK is unable to detecred changes between 'unset' and 'false' (sdk limitation).
+		if !val.(bool) {


hasChanges works well when it's 'true'. So just the 'false' use-case.

TomerHeber · 2024-07-13T16:01:28Z

env0/resource_environment.go

@@ -884,6 +909,11 @@ func getCreatePayload(d *schema.ResourceData, apiClient client.ApiClientInterfac
 		return client.EnvironmentCreate{}, diag.Errorf("schema resource data deserialization failed: %v", err)
 	}

+	//lint:ignore SA1019 reason: https://github.com/hashicorp/terraform-plugin-sdk/issues/817
+	if val, exists := d.GetOkExists("vcs_pr_comments_enabled"); exists {


if the schema is set with 'vcs_pr_comments_enabled' use the value for creation.
(Ignore the global policy).

TomerHeber · 2024-07-13T16:02:58Z

env0/resource_environment.go

@@ -1004,6 +1034,12 @@ func getUpdatePayload(d *schema.ResourceData) (client.EnvironmentUpdate, diag.Di
 		return client.EnvironmentUpdate{}, diag.Errorf("schema resource data deserialization failed: %v", err)
 	}

+	// Because the terraform SDK is unable to detecred changes between 'unset' and 'false' (sdk limitation): always set a value here (even if there's no change).
+	//lint:ignore SA1019 reason: https://github.com/hashicorp/terraform-plugin-sdk/issues/817
+	if val, exists := d.GetOkExists("vcs_pr_comments_enabled"); exists {


if the schema is set with 'vcs_pr_comments_enabled' use the value for update.
(Ignore the global policy).

yaronya · 2024-07-14T12:00:30Z

env0/resource_environment_test.go

@@ -193,6 +215,84 @@ func TestUnitEnvironmentResource(t *testing.T) {
 			})
 		})

+		t.Run("vcs pr comments enabled", func(t *testing.T) {


Can we specifically also test the scenario where vcs_commands_alias is sent without specifying vcs_pr_comments_enabled ?

I don't think it really matters in these type of tests (since the tests are mocked).
I was asked not handle this use-case in the provider. Instead the backend will handle it.

I we already have a harness test passing an alias without vcs_pr_comments_enabled.

resource "env0_environment" "example" { depends_on = [env0_template_project_assignment.assignment] force_destroy = true name = "environment-${random_string.random.result}" project_id = env0_project.test_project.id template_id = env0_template.template.id configuration { name = "environment configuration variable" value = "value" } approve_plan_automatically = true revision = "master" vcs_commands_alias = "alias" drift_detection_cron = var.second_run ? "*/5 * * * *" : "*/10 * * * *" }

Feat: support new apply-all environment property

a440af9

TomerHeber added the pending final review label Jul 10, 2024

TomerHeber self-assigned this Jul 10, 2024

github-actions bot added feature provider api-client integration-tests labels Jul 10, 2024

TomerHeber commented Jul 10, 2024

View reviewed changes

env0/resource_environment.go Outdated Show resolved Hide resolved

TomerHeber commented Jul 10, 2024

View reviewed changes

fix bug

51af114

sagydr reviewed Jul 10, 2024

View reviewed changes

sagydr approved these changes Jul 10, 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 Jul 10, 2024

razbensimon reviewed Jul 10, 2024

View reviewed changes

razbensimon added work in progress and removed ready to merge PR approved - can be merged once the PR owner is ready labels Jul 11, 2024

TomerHeber and others added 3 commits July 11, 2024 10:14

WIP

889a52d

Merge branch 'main' into feat-apply-all-#890

b96fc84

fixes

277e517

TomerHeber added pending final review and removed work in progress labels Jul 13, 2024

TomerHeber commented Jul 13, 2024

View reviewed changes

yaronya reviewed Jul 14, 2024

View reviewed changes

TomerHeber merged commit 34a1857 into main Jul 15, 2024
16 checks passed

TomerHeber deleted the feat-apply-all-#890 branch July 15, 2024 13:27

	Description: "if 'true' all environments created in this project via the UI will be created with an 'enabled' running VCS PR commands using PR comments. Note: in the provider when creating an environment this is ignored, and must be configured explicitly by setting 'vcs_pr_comments_enabled' to 'true' or setting an alias in 'vcs_commands_alias'.",
	Description: "if 'true' all new environments created in this project will be created with an 'enabled' VCS PR commands using PR comments.",

Feat: support new apply-all environment property #893

Feat: support new apply-all environment property #893

Conversation

TomerHeber commented Jul 10, 2024

Issue & Steps to Reproduce / Feature Request

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

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

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

sagydr 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TomerHeber Jul 13, 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 Jul 13, 2024 •

edited

Loading