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

update google_dataflow_flex_template_job to send sdkpipeline parameters via environment field #9031

Conversation

NickElliot
Copy link
Contributor

@NickElliot NickElliot commented Sep 20, 2023

Fixes hashicorp/terraform-provider-google#14679

Since the above issue was introduced, the flex template job resource has had the problem where fields can be sent via both an environment block and the parameters one -- which could lead to permadiffs due to the environment block fields not being flagged as computed, and would lead to guaranteed errors on max_workers and num_workers because they would be sent via both blocks always even when only included in parameters if the user applied a plan through the permadiff (as integer nils are equivalent to 0 within Terraform).

This has been resolved by:

  • Flagging all environment block fields as computed (which while resulting in testing complications, reflects their actual functionality with the parameter block)
  • Customize diff to synchronize parameter fields and their corresponding environment{} variable
  • Creating an error message if users attempt to update fields in the environment{} variable if the corresponding parameter option was also supplied, as an override value being entered for a computed field would lead to a disjointed state due to logic in above bullet being cancelled out. Additionally this would present false functionality in that the create/update commands will always take a parameter-value as the value to be sent.
  • During Create and Update the called resourceDataflowFlexJobSetupEnv() function will now update the ResourceData to assign the parameter values to their corresponding environment variables, and remove those parameters from the parameters block sent to the API.

Plus resolved a bonus undocumented issue

  • Updating max_worker's name within the read command to properly pull its value from the API

This technically will not be a breaking change!

Release Note Template for Downstream PRs (will be copied)

`dataflow`: fixed permadiff when SdkPipeline values are supplied via parameters. 
`dataflow`: fixed max_workers read value permanently displaying as 0.
`dataflow`: fixed issue causing error message when max_workers and num_workers were supplied via parameters.

@NickElliot NickElliot marked this pull request as ready for review September 20, 2023 19:47
@NickElliot
Copy link
Contributor Author

NickElliot commented Sep 20, 2023

the issue comments mentioned a lack of comprehensive documentation on the SdkPipeline options -- I had verified the camelCase naming and other values looking internally, and while this solution is still not comprehensive, it does cover all the environment block variables -- which were what was causing the main issue here. If any more are added that duplicate something that can be supplied via parameters, they will need to be added to the custom code blocks associated with this PR.

@modular-magician
Copy link
Collaborator

Hi there, I'm the Modular magician. I've detected the following information about your changes:

Diff report

Your PR generated some diffs in downstreams - here they are.

Terraform Beta: Diff ( 1 file changed, 341 insertions(+), 20 deletions(-))

@modular-magician
Copy link
Collaborator

Tests analytics

Total tests: 3068
Passed tests 2759
Skipped tests: 307
Affected tests: 2

Action taken

Found 2 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests
TestAccAlloydbInstance_createInstanceWithNetworkConfigAndAllocatedIPRange|TestAccVertexAIIndexEndpoint_updated

Get to know how VCR tests work

@modular-magician
Copy link
Collaborator

$\textcolor{red}{\textsf{Tests failed during RECORDING mode:}}$
TestAccAlloydbInstance_createInstanceWithNetworkConfigAndAllocatedIPRange[Error message] [Debug log]
TestAccVertexAIIndexEndpoint_updated[Error message] [Debug log]

$\textcolor{red}{\textsf{Please fix these to complete your PR.}}$
View the build log or the debug log for each test

@NickElliot
Copy link
Contributor Author

Unrelated test failures :)

Copy link
Member

@rileykarson rileykarson left a comment

Choose a reason for hiding this comment

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

Note that we'll want to test 1 or more flows that would have previously failed. I suggested a specific one inline, but other edge cases should be tested too.

return err
}

if p, ok := d.GetOk("parameters.numWorkers"); 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 you create a test that successfully performs an update? I think we'll hit the error case on any num_workers change, because O+C fields will return a value to d.GetOk. I may be missing something, though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had tested these the most locally since the integers had the funkiest interaction with everything due to Terraform treating a null integer as 0 -- will move to include them more specifically in the update tests

Unless I'm misunderstanding the question --this is why the following line inside checks for if d.HasChange("num_workers") , rather than GetOk("num_workers"), the O+C value will not be flagged as having a change when customizediff runs unless it is also in the user's configuration (with a value changed from its output read value). If the parameter has been removed from the parameters{} block entirely, then this check to perform a value override is skipped and the rest of the resource update will using num_workers as exactly as configured within the user's .tf file (or by persisting the existing computed value if it is never configured).

Copy link
Member

Choose a reason for hiding this comment

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

Thanks!

@NickElliot
Copy link
Contributor Author

/gcbrun

@modular-magician
Copy link
Collaborator

Hi there, I'm the Modular magician. I've detected the following information about your changes:

Diff report

Your PR generated some diffs in downstreams - here they are.

Terraform Beta: Diff ( 2 files changed, 224 insertions(+), 23 deletions(-))

@NickElliot
Copy link
Contributor Author

NickElliot commented Sep 22, 2023

output.log
In the interest of not delaying review for CI run, here is the revised test passing locally

Tests the scenarios of swapping from parameters -> normal field in config alongside value update and verifying that the int parameters do not double send when updating within parameters

return err
}

if p, ok := d.GetOk("parameters.numWorkers"); ok {
Copy link
Member

Choose a reason for hiding this comment

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

Thanks!

@modular-magician
Copy link
Collaborator

Tests analytics

Total tests: 3073
Passed tests 2766
Skipped tests: 306
Affected tests: 1

Action taken

Found 1 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests
TestAccContainerNodePool_withSandboxConfig

Get to know how VCR tests work

@modular-magician
Copy link
Collaborator

$\textcolor{red}{\textsf{Tests failed during RECORDING mode:}}$
TestAccContainerNodePool_withSandboxConfig[Error message] [Debug log]

$\textcolor{red}{\textsf{Please fix these to complete your PR.}}$
View the build log or the debug log for each test

@shermanericts
Copy link

thank you!

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.

4 participants