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

Fix proposed new unknown blocks #2060

Merged
merged 16 commits into from
Jun 6, 2024
Merged

Conversation

VenelinMartinov
Copy link
Contributor

@VenelinMartinov VenelinMartinov commented Jun 5, 2024

This updates our objchange algorithm with the newest one from the OpenTofu repo. This fixes an issue with the way unknown sets are presented to TF.

fixes #1951

@VenelinMartinov VenelinMartinov changed the base branch from master to vvm/unknown_tests June 5, 2024 11:14
Copy link

codecov bot commented Jun 5, 2024

Codecov Report

Attention: Patch coverage is 15.86207% with 488 lines in your changes missing coverage. Please review.

Project coverage is 61.20%. Comparing base (ba48631) to head (b6947ee).

Files Patch % Lines
...m/sdk-v2/internal/tf/plans/objchange/plan_valid.go 1.21% 324 Missing and 1 partial ⚠️
...im/sdk-v2/internal/tf/plans/objchange/objchange.go 40.93% 119 Missing and 8 partials ⚠️
...im/sdk-v2/internal/tf/configs/configschema/path.go 0.00% 31 Missing ⚠️
...rnal/tf/configs/configschema/nestingmode_string.go 0.00% 5 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2060      +/-   ##
==========================================
- Coverage   61.57%   61.20%   -0.37%     
==========================================
  Files         334      337       +3     
  Lines       44955    45177     +222     
==========================================
- Hits        27681    27651      -30     
- Misses      15750    16005     +255     
+ Partials     1524     1521       -3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@@ -69,7 +69,10 @@ func PlannedDataResourceObject(schema *configschema.Block, config cty.Value) cty
}

func proposedNew(schema *configschema.Block, prior, config cty.Value) cty.Value {
if config.IsNull() || !config.IsKnown() {
if !config.IsKnown() {
Copy link
Member

Choose a reason for hiding this comment

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

This file is marked with:

// Code copied from https://github.com/hashicorp/terraform.git by go generate; DO NOT EDIT.

Are you sure this change brings the overall system closer to TF Behavior? Can we place it outside this file to keep it vendored?

Copy link
Member

Choose a reason for hiding this comment

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

Ah I see, I see, I'd guess this branch is never even hit in TF. So it's fine to edit. If you can place the edit at the call site of ObjChange not this file itself it can be easier to keep up with vendoring. Thank you!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The function is recursive, so we can't do that easily. We could add a reconstruction step after this function which goes and recovers unknowns where they were discarded but I feel like this is better.

I'll see if there's a newer version of this upstream.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup, updating the objchange copy did fix the issue.

Base automatically changed from vvm/unknown_tests to master June 5, 2024 14:22
const terraformRepo = "https://github.com/hashicorp/terraform.git"
const terraformVer = "v1.3.9"
const terraformRepo = "https://github.com/opentofu/opentofu.git"
const terraformVer = "v1.7.2"
Copy link
Member

Choose a reason for hiding this comment

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

Amazing.

@t0yv0
Copy link
Member

t0yv0 commented Jun 5, 2024

The objchange update is amazing. This can be a very big change now may trip up downstream tests, though probably a good one. Great find!!

@VenelinMartinov VenelinMartinov marked this pull request as ready for review June 6, 2024 14:51
Copy link
Member

@t0yv0 t0yv0 left a comment

Choose a reason for hiding this comment

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

This looks good but please relabel the commit message so commit log clearly states what's changing (we're updating ObjChange algorithm to the new opentofu version).

Copy link
Member

@iwahbe iwahbe left a comment

Choose a reason for hiding this comment

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

Looks like we are missing a test fix, but otherwise the change looks great.

@VenelinMartinov
Copy link
Contributor Author

VenelinMartinov commented Jun 6, 2024

Well, this passes for me locally...


Had to merge master - apparently CI does not run on your branch but on the merged result.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

required resource update due to changes in nested attributes that have not been modified
3 participants