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

Jump to bottom

Fix: vcs_command_alias should be set to "" when vcs_pr_comments_enabl… #965

Merged
merged 2 commits into from
Oct 29, 2024
Merged

Fix: vcs_command_alias should be set to "" when vcs_pr_comments_enabl… #965

merged 2 commits into from
Oct 29, 2024

Conversation

TomerHeber
Copy link
Collaborator

…ed is disabled

Issue & Steps to Reproduce / Feature Request

fixes #964

Solution

Forces alias to be empty if vcs_pr_comments_enabled is unset or false.
Updated acceptance tests.

@TomerHeber TomerHeber self-assigned this Oct 21, 2024
TomerHeber
TomerHeber commented Oct 21, 2024
View reviewed changes
env0/resource_environment.go
@@ -1062,6 +1062,11 @@ func getCreatePayload(d *schema.ResourceData, apiClient client.ApiClientInterfac
}
}

// Send an empty vcs_commands_alias if vcs_pr_comments_enabled is 'false' or unset - https://github.com/env0/terraform-provider-env0/issues/964
if payload.VcsPrCommentsEnabled == nil || !*payload.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.

create use-case.

TomerHeber
TomerHeber commented Oct 21, 2024
View reviewed changes
env0/resource_environment.go
@@ -1129,6 +1134,11 @@ func getUpdatePayload(d *schema.ResourceData) (client.EnvironmentUpdate, diag.Di
payload.IsArchived = boolPtr(d.Get("is_inactive").(bool))
}

// Send an empty vcs_commands_alias if vcs_pr_comments_enabled is 'false' or unset - https://github.com/env0/terraform-provider-env0/issues/964
if payload.VcsPrCommentsEnabled == nil || !*payload.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.

update use-case.

TomerHeber
TomerHeber commented Oct 21, 2024
View reviewed changes
env0/resource_environment_test.go
@@ -59,7 +59,7 @@ func TestUnitEnvironmentResource(t *testing.T) {
},
TerragruntWorkingDirectory: "/terragrunt/directory/",
VcsCommandsAlias: "alias2",
VcsPrCommentsEnabled: false,
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.

in some places this had been set to true, so alias will be passed...

@TomerHeber
Copy link
Collaborator Author

/review

@bot-env0 bot-env0 requested a review from a team October 21, 2024 23:24
Yossi-kerner
Yossi-kerner reviewed Oct 27, 2024
View reviewed changes
Copy link
Contributor

@Yossi-kerner Yossi-kerner left a comment

Choose a reason for hiding this comment

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

I see that you had to add the VcsPrCommentsEnabled field set to true. Does that mean that users that don't have that prop and depends on having an alias or not having it to enable and disable pr comments will have issues no?

@TomerHeber
Copy link
Collaborator Author

I see that you had to add the VcsPrCommentsEnabled field set to true. Does that mean that users that don't have that prop and depends on having an alias or not having it to enable and disable pr comments will have issues no?

Hi @Yossi-kerner - valid question!
The "true" was set in tests, many of the tests were broken becuase of the change in the code. For the alias to be "set" I have to also set VcsPrCommentsEnabled to 'true'. So this was a minimum change to fix the tests (Which unfortunatly, took some time... because most tests got broken...).

To be honest, the fix could have been in the backend and not in the provider. This is an anti-pattern for the provider code. However, I was asked to make the fix in the provider. So that's what I did (:

Yossi-kerner
Yossi-kerner approved these changes Oct 29, 2024
View reviewed changes
Copy link
Contributor

@Yossi-kerner Yossi-kerner left a comment

Choose a reason for hiding this comment

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

Sorry for the late approve. We decided we are ok with that change, just make sure to change the major version when we release because it's a breaking change and make it clear in the change log what should be done 🙏 Thanks

@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 Oct 29, 2024
@TomerHeber TomerHeber merged commit 9a78b65 into main Oct 29, 2024
5 checks passed
@TomerHeber TomerHeber deleted the fix-vcs-#964 branch October 29, 2024 16:00
@TomerHeber
Copy link
Collaborator Author

Sorry for the late approve. We decided we are ok with that change, just make sure to change the major version when we release because it's a breaking change and make it clear in the change log what should be done 🙏 Thanks

Feel free to edit the description if it's unclear.
https://github.com/env0/terraform-provider-env0/releases/tag/v1.22.0

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Yossi-kerner Yossi-kerner Yossi-kerner approved these changes

Assignees

@TomerHeber TomerHeber

Labels
fix provider ready to merge PR approved - can be merged once the PR owner is ready
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

vcs_command_alias should be set to "" when vcs_pr_comments_enabled is disabled
2 participants
@TomerHeber @Yossi-kerner