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.

Sign up for GitHub

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

Fix: approve_plan_automatically attribute fails for sub_environment #954

Merged
merged 6 commits into from
Sep 15, 2024
Merged

Fix: approve_plan_automatically attribute fails for sub_environment #954

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
6604c6f
Fix: approve_plan_automatically attribute fails for sub_environment
TomerHeber Sep 11, 2024
8cf54b9
fix
TomerHeber Sep 11, 2024
152b03f
hack
TomerHeber Sep 11, 2024
25ddb1a
re-use code
TomerHeber Sep 15, 2024
5c3d871
Merge branch 'main' into fix-workflow-approve-plan-#947
TomerHeber Sep 15, 2024
b1d22bb
add test case
TomerHeber Sep 15, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 18 additions & 1 deletion env0/resource_environment.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -415,7 +415,8 @@ func setEnvironmentSchema(ctx context.Context, d *schema.ResourceData, environme
d.Set("output", string(environment.LatestDeploymentLog.Output))
}

if environment.RequiresApproval != nil {
// Don't update this value for workflow environments - this value should always be 'false'.
if _, isWorkflow := d.GetOk("sub_environment_configuration"); !isWorkflow && environment.RequiresApproval != nil {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

don't modify the schema for workflow environment...
generally this would not be required... however due to the change in line 1195 (which I believe is a backend issue) - I had to add this here.

(see my comment in line 1195).

d.Set("approve_plan_automatically", !*environment.RequiresApproval)
}

Expand Down Expand Up @@ -976,9 +977,20 @@ func getCreatePayload(d *schema.ResourceData, apiClient client.ApiClientInterfac
payload.AutoDeployOnPathChangesOnly = boolPtr(val.(bool))
}

_, isWorkflow := d.GetOk("sub_environment_configuration")

//lint:ignore SA1019 reason: https://github.com/hashicorp/terraform-plugin-sdk/issues/817
if val, exists := d.GetOkExists("approve_plan_automatically"); exists {
payload.RequiresApproval = boolPtr(!val.(bool))

if isWorkflow && *payload.RequiresApproval {
return client.EnvironmentCreate{}, diag.Errorf("approve_plan_automatically cannot be 'false' for workflows")
}
}

// For 'Workflows', the 'root' environment should never require an approval.
if _, ok := d.GetOk("sub_environment_configuration"); ok {
Copy link
Member

Choose a reason for hiding this comment

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

can we reuse the isWorkflow from line 980?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

thanks! fixed!

payload.RequiresApproval = boolPtr(false)
Copy link
Member

Choose a reason for hiding this comment

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

how does this if block is different than the one above?
I'm a bit confused

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

line 986 - is the use-case when someone excplicitly set 'approve_plan_automatically' to 'false' and it's a workflow - in that case we can quickly error out as bad schema set by the user.

line 992 - is the use-case where it's a workflow - the value is forced to be 'false'.

}

//lint:ignore SA1019 reason: https://github.com/hashicorp/terraform-plugin-sdk/issues/817
Expand Down Expand Up @@ -1179,6 +1191,11 @@ func getDeployPayload(d *schema.ResourceData, apiClient client.ApiClientInterfac
payload.BlueprintRevision = revision.(string)
}

// For 'Workflows', the 'root' environment should never require a user approval.
if _, ok := d.GetOk("sub_environment_configuration"); ok {
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 shouldn't be needed...
This might be a bug in the backend...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

it seems that the backend will error if this is 'undefined'.

Copy link

Choose a reason for hiding this comment

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

this could be overridden by a project policy, this is a good extra step anyways.

payload.UserRequiresApproval = boolPtr(false)
}

if isRedeploy {
if revision, ok := d.GetOk("without_template_settings.0.revision"); ok {
payload.BlueprintRevision = revision.(string)
Expand Down
23 changes: 13 additions & 10 deletions env0/resource_environment_test.go
View file
Open in desktop
Copy link
Member

Choose a reason for hiding this comment

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

Can we add a test of wrong terraform configuration where customer put approve_plan_automatically = false?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

added b1d22bb

Original file line number Diff line number Diff line change
Expand Up @@ -2888,11 +2888,11 @@ func TestUnitEnvironmentWithSubEnvironment(t *testing.T) {
subEnvrionmentWithId.Id = workflowSubEnvironment.EnvironmentId

environment := client.Environment{
Id: "id",
Name: "environment",
ProjectId: "project-id",
BlueprintId: "template-id",

Id: "id",
Name: "environment",
ProjectId: "project-id",
BlueprintId: "template-id",
RequiresApproval: boolPtr(false),
LatestDeploymentLog: client.DeploymentLog{
WorkflowFile: &client.WorkflowFile{
Environments: map[string]client.WorkflowSubEnvironment{
Expand All @@ -2912,8 +2912,9 @@ func TestUnitEnvironmentWithSubEnvironment(t *testing.T) {
}

environmentCreatePayload := client.EnvironmentCreate{
Name: environment.Name,
ProjectId: environment.ProjectId,
Name: environment.Name,
ProjectId: environment.ProjectId,
RequiresApproval: boolPtr(false),
ConfigurationChanges: &client.ConfigurationChanges{
{
Name: "n1",
Expand All @@ -2929,7 +2930,8 @@ func TestUnitEnvironmentWithSubEnvironment(t *testing.T) {
},
},
DeployRequest: &client.DeployRequest{
BlueprintId: environment.BlueprintId,
BlueprintId: environment.BlueprintId,
UserRequiresApproval: boolPtr(false),
SubEnvironments: map[string]client.SubEnvironment{
subEnvironment.Alias: {
Workspace: subEnvironment.Workspace,
Expand All @@ -2946,8 +2948,9 @@ func TestUnitEnvironmentWithSubEnvironment(t *testing.T) {
}

deployRequest := client.DeployRequest{
BlueprintId: environment.BlueprintId,
BlueprintRevision: environment.LatestDeploymentLog.BlueprintRevision,
BlueprintId: environment.BlueprintId,
BlueprintRevision: environment.LatestDeploymentLog.BlueprintRevision,
UserRequiresApproval: boolPtr(false),
SubEnvironments: map[string]client.SubEnvironment{
subEnvironment.Alias: {
Revision: subEnvironment.Revision,
Expand Down
3 changes: 1 addition & 2 deletions tests/integration/012_environment/main.tf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -257,7 +257,6 @@ resource "env0_environment" "workflow-environment" {
name = "environment-workflow-${random_string.random.result}"
project_id = env0_project.test_project.id
template_id = env0_template.workflow_template.id
approve_plan_automatically = true

configuration {
name = "n1"
Expand All @@ -269,7 +268,7 @@ resource "env0_environment" "workflow-environment" {
sub_environment_configuration {
alias = "rootService1"
revision = "master"
approve_plan_automatically = var.second_run ? true : false
approve_plan_automatically = var.second_run ? false : true
configuration {
name = "sub_env1_var1"
value = "hello"
Expand Down
Loading