[TF-10830] add new attribute to org resource and data blocks #1169

mjyocca · 2023-12-11T23:12:15Z

Description

Adding new organization attribute, aggregated_commit_status_enabled to organization data/resource blocks.

Remember to:

Update the Change Log
Update the Documentation

Testing plan

Create a tfe_organization resource with the required arguments: name, email.
Add the new aggregated_commit_status_enabled argument, then and apply the changes
Verify against target TF host that is has been updated to reflect the assigned value: true, false

Example config:

resource "tfe_organization" "my_org" {
  email = "testemail@test.com"
  name = "test-org"
  aggregated_commit_status_enabled = true
}

External links

Include any links here that might be helpful for people reviewing your PR. If there are none, feel free to delete this section.

Output from acceptance tests

Please run applicable acceptance tests locally and include the output here. See testing.md to learn how to run acceptance tests.

If you are an external contributor, your contribution(s) will first be reviewed before running them against the project's CI pipeline.

$ TESTARGS="-run TestAccTFEOrganization_update_costEstimation" make testacc                            
TF_ACC=1 TF_LOG_SDK_PROTO=OFF go test $(go list ./... |grep -v 'vendor') -v -run TestAccTFEOrganization_update_costEstimation -timeout 15m
?       github.com/hashicorp/terraform-provider-tfe     [no test files]
testing: warning: no tests to run
PASS
ok      github.com/hashicorp/terraform-provider-tfe/internal/client     0.227s [no tests to run]
testing: warning: no tests to run
PASS
ok      github.com/hashicorp/terraform-provider-tfe/internal/logging    0.306s [no tests to run]
?       github.com/hashicorp/terraform-provider-tfe/version     [no test files]
=== RUN   TestAccTFEOrganization_update_costEstimation
    resource_tfe_organization_test.go:118: Skipping this test until the SDK can support importing resources before applying a configuration
--- SKIP: TestAccTFEOrganization_update_costEstimation (0.00s)
PASS
ok      github.com/hashicorp/terraform-provider-tfe/internal/provider   0.530s

TESTARGS="-run TestAccTFEOrganization_update_costEstimation" make testacc
TF_ACC=1 TF_LOG_SDK_PROTO=OFF go test $(go list ./... |grep -v 'vendor') -v -run TestAccTFEOrganization_update_costEstimation -timeout 15m
?       github.com/hashicorp/terraform-provider-tfe     [no test files]
testing: warning: no tests to run
PASS
ok      github.com/hashicorp/terraform-provider-tfe/internal/client     0.314s [no tests to run]
testing: warning: no tests to run
PASS
ok      github.com/hashicorp/terraform-provider-tfe/internal/logging    0.150s [no tests to run]
?       github.com/hashicorp/terraform-provider-tfe/version     [no test files]
=== RUN   TestAccTFEOrganization_update_costEstimation
    resource_tfe_organization_test.go:118: Skipping this test until the SDK can support importing resources before applying a configuration
--- SKIP: TestAccTFEOrganization_update_costEstimation (0.00s)
PASS
ok      github.com/hashicorp/terraform-provider-tfe/internal/provider   0.538s

website/docs/d/organization.html.markdown

lilincmu

Except for one question, the rest looks good to me!

internal/provider/resource_tfe_organization.go

mjyocca · 2024-01-29T15:51:10Z

internal/provider/resource_tfe_organization.go

+	if d.HasChange("aggregated_commit_status_enabled") {
+		_, newVal := d.GetChange("aggregated_commit_status_enabled")
+		options.AggregatedCommitStatusEnabled = tfe.Bool(newVal.(bool))
 	}


Due to issues withGetOk and resolving zero value for booleans, workaround to only include attribute if it has been altered. Prevents issues with aggregated_commit_status_enabled = true changed to aggregated_commit_status_enabled = false. Whereas it was previously omitted.

This workaround makes sense to me! I think it's more readable than using GetOk with an else block, which confused me a bit at first.

lilincmu

Looks good to me! 👍

brandonc · 2024-02-01T16:47:48Z

website/docs/r/organization.html.markdown

@@ -35,6 +35,7 @@ The following arguments are supported:
 * `owners_team_saml_role_id` - (Optional) The name of the "owners" team.
 * `cost_estimation_enabled` - (Optional) Whether or not the cost estimation feature is enabled for all workspaces in the organization. Defaults to true. In a Terraform Cloud organization which does not have Teams & Governance features, this value is always false and cannot be changed. In Terraform Enterprise, Cost Estimation must also be enabled in Site Administration.
 * `send_passing_statuses_for_untriggered_speculative_plans` - (Optional) Whether or not to send VCS status updates for untriggered speculative plans. This can be useful if large numbers of untriggered workspaces are exhausting request limits for connected version control service providers like GitHub. Defaults to false. In Terraform Enterprise, this setting has no effect and cannot be changed but is also available in Site Administration.
+* `aggregated_commit_status_enabled` - (Optional) Whether or not to enable Aggregated Status Checks. This can be useful for monorepo repositories with multiple workspaces receiving status checks for events such as a pull request. If enabled, `send_passing_statuses_for_untriggered_speculative_plans` needs to be false.


This could use a validation in order to catch it during the plan phase

One potential way to accomplish this would be to add ConflictsWith: []string{"send_passing_statuses_..."} to the aggregated_commit_status_enabled attribute.

I was not sure if we wanted to duplicate validation in both places but I don't mind if thats the direction we want to move in.

sebasslash

This LGTM 😃

sebasslash · 2024-02-01T20:26:40Z

website/docs/r/organization.html.markdown

@@ -35,6 +35,7 @@ The following arguments are supported:
 * `owners_team_saml_role_id` - (Optional) The name of the "owners" team.
 * `cost_estimation_enabled` - (Optional) Whether or not the cost estimation feature is enabled for all workspaces in the organization. Defaults to true. In a Terraform Cloud organization which does not have Teams & Governance features, this value is always false and cannot be changed. In Terraform Enterprise, Cost Estimation must also be enabled in Site Administration.
 * `send_passing_statuses_for_untriggered_speculative_plans` - (Optional) Whether or not to send VCS status updates for untriggered speculative plans. This can be useful if large numbers of untriggered workspaces are exhausting request limits for connected version control service providers like GitHub. Defaults to false. In Terraform Enterprise, this setting has no effect and cannot be changed but is also available in Site Administration.
+* `aggregated_commit_status_enabled` - (Optional) Whether or not to enable Aggregated Status Checks. This can be useful for monorepo repositories with multiple workspaces receiving status checks for events such as a pull request. If enabled, `send_passing_statuses_for_untriggered_speculative_plans` needs to be false.


One potential way to accomplish this would be to add ConflictsWith: []string{"send_passing_statuses_..."} to the aggregated_commit_status_enabled attribute.

mjyocca · 2024-02-01T20:59:59Z

One potential way to accomplish this would be to add ConflictsWith: []string{"send_passing_statuses_..."} to the aggregated_commit_status_enabled attribute.

@sebasslash Great callout! I looked into this however we want to be able to allow including both attributes, but just not both assigned to true. Not sure if ConflictsWith allows to specify conflicting keys with their values?

mjyocca force-pushed the mjyocca/TF-10830 branch 2 times, most recently from 9529242 to a025e00 Compare January 24, 2024 20:49

mjyocca marked this pull request as ready for review January 25, 2024 16:44

mjyocca requested a review from a team as a code owner January 25, 2024 16:44

mjyocca requested a review from lilincmu January 25, 2024 16:47

ctrombley reviewed Jan 25, 2024

View reviewed changes

website/docs/d/organization.html.markdown Outdated Show resolved Hide resolved

lilincmu reviewed Jan 26, 2024

View reviewed changes

internal/provider/resource_tfe_organization.go Outdated Show resolved Hide resolved

mjyocca commented Jan 29, 2024

View reviewed changes

mjyocca requested review from ctrombley and lilincmu January 29, 2024 16:56

lilincmu approved these changes Jan 29, 2024

View reviewed changes

mjyocca force-pushed the mjyocca/TF-10830 branch 2 times, most recently from 5478d16 to ebe256b Compare January 30, 2024 20:13

mjyocca requested a review from a team January 30, 2024 20:55

mjyocca force-pushed the mjyocca/TF-10830 branch 2 times, most recently from dd71d99 to 30c284c Compare February 1, 2024 15:10

brandonc approved these changes Feb 1, 2024

View reviewed changes

sebasslash approved these changes Feb 1, 2024

View reviewed changes

mjyocca added 9 commits February 2, 2024 07:58

TF-10830 add new attribute to organization resource and data block

9824181

fix: wrong test expectation

c206dcb

doc: add changelog entry for change

e9889ec

fix: remove conflicts with

2f58e1a

fix: scenarios AggregatedCommitStatusEnabled is not updated

53f5655

doc: update data and resource docs

08be575

docs: omit conflicting assignment mention

9669a77

fix: removed message from wrong page

646ab7f

refactor: instrument HasChange and GetChange

a6e26ab

mjyocca force-pushed the mjyocca/TF-10830 branch from 30c284c to a6e26ab Compare February 2, 2024 16:00

mjyocca merged commit e5854c7 into main Feb 2, 2024
9 checks passed

mjyocca deleted the mjyocca/TF-10830 branch February 2, 2024 16:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[TF-10830] add new attribute to org resource and data blocks #1169

[TF-10830] add new attribute to org resource and data blocks #1169

mjyocca commented Dec 11, 2023 •

edited

Loading

lilincmu left a comment

mjyocca Jan 29, 2024

lilincmu Jan 29, 2024

lilincmu left a comment

brandonc Feb 1, 2024

sebasslash Feb 1, 2024

mjyocca Feb 1, 2024

sebasslash left a comment

sebasslash Feb 1, 2024

mjyocca commented Feb 1, 2024

[TF-10830] add new attribute to org resource and data blocks #1169

[TF-10830] add new attribute to org resource and data blocks #1169

Conversation

mjyocca commented Dec 11, 2023 • edited Loading

Description

Testing plan

External links

Output from acceptance tests

lilincmu left a comment

Choose a reason for hiding this comment

mjyocca Jan 29, 2024

Choose a reason for hiding this comment

lilincmu Jan 29, 2024

Choose a reason for hiding this comment

lilincmu left a comment

Choose a reason for hiding this comment

brandonc Feb 1, 2024

Choose a reason for hiding this comment

sebasslash Feb 1, 2024

Choose a reason for hiding this comment

mjyocca Feb 1, 2024

Choose a reason for hiding this comment

sebasslash left a comment

Choose a reason for hiding this comment

sebasslash Feb 1, 2024

Choose a reason for hiding this comment

mjyocca commented Feb 1, 2024

mjyocca commented Dec 11, 2023 •

edited

Loading