Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feat: support new apply-all environment property #893

Merged
merged 5 commits into from
Jul 15, 2024

Conversation

TomerHeber
Copy link
Collaborator

Issue & Steps to Reproduce / Feature Request

resolves #890

Solution

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

@@ -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:"-"`
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

special handling to avoid drifts.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

  1. use the the provider version
  2. do not set vcsPrCommentsEnabled
  3. 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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

@@ -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 {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

@@ -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
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

oh! interesting...

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

@@ -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
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

@@ -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'.",
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

going to make changes based on our discussion.

Copy link
Contributor

Choose a reason for hiding this comment

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

@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.",

Copy link
Contributor

Choose a reason for hiding this comment

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

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)

Copy link
Contributor

Choose a reason for hiding this comment

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

let me know if we didnt understand you

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

skip_apply_when_plan_is_empty = true
disable_destroy_environments = true
skip_redundant_deployments = true
vcs_pr_comments_enabled_default = true
Copy link
Contributor

Choose a reason for hiding this comment

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

This tests the project policy configuration?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@sagydr sagydr left a comment

Choose a reason for hiding this comment

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

looks good, approved with minor comments/questions

@github-actions 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
@@ -18,6 +18,7 @@ type Policy struct {
ForceRemoteBackend bool `json:"forceRemoteBackend"`
DriftDetectionCron string `json:"driftDetectionCron"`
DriftDetectionEnabled bool `json:"driftDetectionEnabled"`
VcsPrCommentsEnabledDefault bool `json:"vcsPrCommentsEnabledDefault"`
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Comment on lines 269 to 271
"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'.",
Copy link
Contributor

Choose a reason for hiding this comment

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

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'.",

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

sure!

@razbensimon 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
@@ -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:"-"`
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

@@ -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:"-"`
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

"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,
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

@@ -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 {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

}

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

Choose a reason for hiding this comment

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

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.

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) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

@@ -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 {
Copy link
Collaborator Author

@TomerHeber TomerHeber Jul 13, 2024

Choose a reason for hiding this comment

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

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

@@ -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 {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

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

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

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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 * * * *"
}

@TomerHeber TomerHeber merged commit 34a1857 into main Jul 15, 2024
16 checks passed
@TomerHeber TomerHeber deleted the feat-apply-all-#890 branch July 15, 2024 13:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support new apply-all environment property
4 participants