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(work pools): fix work pool base job template "inconsistent values" #249

Merged
merged 7 commits into from
Aug 23, 2024

Conversation

mitchnielsen
Copy link
Contributor

@mitchnielsen mitchnielsen commented Aug 23, 2024

Summary

  • Addresses the "inconsistent values" error the provider would sometimes return when working with work pools that specify a base job template (context: Inconsistent result after apply bug with prefect_work_pool #244 (comment))
  • Adds base_job_template to the Work Pool tests
  • Conforms to the newer, simpler method of unmarshaling the base job template that we use in our Block data logic

More information on each change is available in the relevant commit message.

Closes #244

Copies the simpler way of decoding JSON that we use for Blocks.
The base_job_template in the plan can differ from the base_job_template
in the work pool object retrieved from the API, specifically in the
order of the fields. In these cases, the objects are functionally
equivalent, but Terraform returns an error because it retrieved an
"inconsistent result after apply."

This change accounts for those differences by comparing the objects and
checking for equality using the go-cmp package. If the objects are not
equal, we will return an error diagnostic.
The BaseJobTemplate field is large due to the number of required fields.
As a result, when there's a failure, printing the entire struct twice
(once for expected, once for got) is very hard to parse visually -
especially because Terraform logs them as JSON objects with quotes
escaped and with everything on one "line."

This package wraps go-cmp, which was used for comparison in the previous
commit. It adds on the ability to return the differences as a string,
which is much more readable in the output.

As a contrived example, when the resource retrieved from the API has
extra fields:

```
prefect_work_pool.example_work_pool Modifying...
╷
│ Error: Unexpected difference between base_job_template
│
│ Expected the provided base_job_template to be equal to the one retrieved from the API, differences: {
│     "prop-added":{"job_configuration": {}
│         "prop-added":{"auto_deregister_task_definition": "{{ auto_deregister_task_definition }}",}
│         "prop-added":{"cloudwatch_logs_options": "{{ cloudwatch_logs_options }}",}
```
Ensures that the base_job_template field is tested in the work pool
tests.

Note that we don't use the TestCheckResourceAttr check because it can't
account for differences in the JSON object ordering.
internal/provider/resources/work_pool.go Outdated Show resolved Hide resolved
internal/provider/resources/work_pool.go Outdated Show resolved Hide resolved
internal/provider/resources/work_pool.go Outdated Show resolved Hide resolved
internal/provider/resources/work_pool_test.go Show resolved Hide resolved
@mitchnielsen mitchnielsen marked this pull request as ready for review August 23, 2024 04:00
@mitchnielsen mitchnielsen requested a review from a team as a code owner August 23, 2024 04:00
This package allows comparing interfaces, so it's more flexible and
requires less conversion between types.

Since it's more flexible, also moves the function to a helper so we can
use it to compare other objects in the provider in the future.

Example of the output when the objects do not map:

```
=== RUN   TestAccResource_work_pool
    work_pool_test.go:58: Step 1/6 error: Check failed: Check 2/5 error: Found unexpected differences in work pool base_job_template: map[job_configuration]: map[command:{{ command }} env:{{ env }} labels:{{ labels }} name:{{ name }} stream_output:{{ stream_output }} working_dir:{{ working_dir }}] != <does not have key>
  ```
// If the base job template from the retrieved work pool is equal
// to the base job template from the plan (taking into account that the
// fields could be in a different order), then we will use the plan's definition
// of the base job template in the state. This avoids "inconsistent value"
Copy link
Contributor

Choose a reason for hiding this comment

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

smart

Co-authored-by: Edward Park <ed.sh.park@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix Fixing a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Inconsistent result after apply bug with prefect_work_pool
2 participants