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

Enable disabled to be set by an interpolated value or from a module value. #36

Merged
merged 8 commits into from
Mar 7, 2025

Conversation

nicholasjackson
Copy link
Contributor

Previously if a resource had disabled set by using a variable:

variable "disabled" {
  value = true
}

resource "test" "a" {
  disabled = variable.disabled
}

When that variable was overridden in a module

module "a"
  variables = {
    disabled = false
  }

The original value of the variable would be used as disabled was calculated before evaluating the context.

As a bonus effect, it is now possible to set the value of disabled using an interpolated value from another resource or by using a function.

resource "test" "a" {
  disabled = resource.test.b.networkcount > 2 ? true : false
}

@nicholasjackson nicholasjackson added the enhancement New feature or request label Mar 7, 2025
@nicholasjackson nicholasjackson requested review from eveld and Copilot March 7, 2025 09:11
@nicholasjackson nicholasjackson self-assigned this Mar 7, 2025
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

PR Overview

This PR modifies how the disabled state is evaluated and applied for resources, enabling the use of interpolated values and module overrides. Key changes include:

  • Removal of early skipping of disabled resources in the DAG construction.
  • Adjustments to error handling in module resource lookups to ignore errors when resources may be disabled.
  • Updates to tests and resource lookup formatting with a new helper function.

Reviewed Changes

File Description
dag.go Revised disabled evaluation and dependency resolution in the DAG logic.
parse_test.go Updated expected resource counts and added tests for module disabled override.
resources/fqrn.go Added helper function (StringWithoutAttribute) for formatting resource IDs.
config.go Changed error return message formatting to use StringWithoutAttribute.
parser.go Removed the redundant setDisabled function to rely on new disabled evaluation logic.

Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (1)

parser.go:544

  • The removal of the setDisabled function may lead to inconsistent handling of the disabled state; please verify that all cases previously covered by setDisabled are now managed by the new evaluation logic in createCallback.
func setDisabled(ctx *hcl.EvalContext, r types.Resource, b *hclsyntax.Body, parentDisabled bool) error {

@nicholasjackson nicholasjackson requested a review from Copilot March 7, 2025 09:25
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

PR Overview

This PR enables the "disabled" property to be set dynamically via interpolated values or module inputs, along with a bonus improvement in allowing expressions and function evaluations for the disabled state.

  • Refactored the logic for processing disabled resources in the DAG construction.
  • Updated tests and resource lookup error messages to support dynamic disabled values.
  • Added utility functions (e.g., StringWithoutAttribute) for cleaner error messaging in resource handling.

Reviewed Changes

File Description
dag.go Adjustments in handling "disabled" evaluation and error message tweaks.
parse_test.go Revised resource count expectations and added test for module override.
resources/fqrn.go Added StringWithoutAttribute to improve error messages and resource lookup.
utils_test.go Updated file location references in tests.
config.go Modified resource lookup error reporting using StringWithoutAttribute.
parser.go Removed redundant disabled handling previously done in resource parsing.

Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.

@nicholasjackson nicholasjackson requested a review from Copilot March 7, 2025 10:24
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

PR Overview

This PR refactors how the "disabled" property is evaluated, enabling it to be set dynamically via interpolated expressions or module values and ensuring that module-dependent resources are correctly processed.

  • Refactored DAG processing logic to compute disabled state through evaluated expressions and removed legacy disabled handling.
  • Updated tests and expected resource counts to reflect the dynamic nature of the disabled property.
  • Modified the FQRN string generation used in error messages to strip attribute details when reporting missing resources.

Reviewed Changes

File Description
dag.go Updated disabled handling and dependency resolution by ignoring errors for module resource lookups.
resources/fqrn.go Added a StringWithoutAttribute method to modify error messaging in resource lookups.
parse_test.go Updated expected resource counts to match the new handling of module and disabled resources.
utils_test.go Adjusted file location parameters for reading test fixture files.
config.go Modified error returns to use StringWithoutAttribute for clearer module error messages.
parser.go Removed legacy setDisabled calls in favor of the new disabled expression evaluation logic.

Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.

Comments suppressed due to low confidence (1)

parser.go:544

  • Removal of legacy setDisabled calls in parser.go necessitates thorough test coverage to ensure that the new expression-based disabled evaluation correctly handles all cases.
func setDisabled(ctx *hcl.EvalContext, r types.Resource, b *hclsyntax.Body, parentDisabled bool) error {

@eveld eveld merged commit e00382b into main Mar 7, 2025
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants