-
Notifications
You must be signed in to change notification settings - Fork 15
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
Conversation
@@ -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:"-"` |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
- 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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
env0/resource_environment.go
Outdated
@@ -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 { |
There was a problem hiding this comment.
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.
env0/resource_environment.go
Outdated
@@ -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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh! interesting...
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
env0/resource_environment.go
Outdated
@@ -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 |
There was a problem hiding this comment.
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.
env0/resource_project_policy.go
Outdated
@@ -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'.", |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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:
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.", |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this 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
client/project_policy.go
Outdated
@@ -18,6 +18,7 @@ type Policy struct { | |||
ForceRemoteBackend bool `json:"forceRemoteBackend"` | |||
DriftDetectionCron string `json:"driftDetectionCron"` | |||
DriftDetectionEnabled bool `json:"driftDetectionEnabled"` | |||
VcsPrCommentsEnabledDefault bool `json:"vcsPrCommentsEnabledDefault"` |
There was a problem hiding this comment.
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...
env0/resource_environment.go
Outdated
"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'.", |
There was a problem hiding this comment.
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
"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'.", |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure!
@@ -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:"-"` |
There was a problem hiding this comment.
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:"-"` |
There was a problem hiding this comment.
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, |
There was a problem hiding this comment.
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 { |
There was a problem hiding this comment.
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 { |
There was a problem hiding this comment.
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) { |
There was a problem hiding this comment.
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 { |
There was a problem hiding this comment.
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 { |
There was a problem hiding this comment.
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) { |
There was a problem hiding this comment.
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
?
There was a problem hiding this comment.
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 * * * *"
}
Issue & Steps to Reproduce / Feature Request
resolves #890
Solution