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(tfe_policy): update query if changed #1108

Closed
wants to merge 4 commits into from

Conversation

skeggse
Copy link
Contributor

@skeggse skeggse commented Oct 18, 2023

Description

I tried to update the query of a policy, and it didn't update - but the state thought it did! 🤦

Remember to:

Testing plan

I'm not going to spend the time to test this independently - it's too much effort. If it builds, someone else can try testing this. It's rather upsetting that this sort of bug is even possible in this Terraform provider.

External links

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.

Feel free to do this.

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

@skeggse skeggse requested a review from a team as a code owner October 18, 2023 22:34
Copy link
Member

@cam-stitt cam-stitt left a comment

Choose a reason for hiding this comment

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

Would be nice to have a test case for this so it never regresses.

Copy link
Contributor

@glennsarti glennsarti left a comment

Choose a reason for hiding this comment

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

Looks good from the compliance team!

Thanks for the PR @skeggse !

Copy link
Member

@nfagerlund nfagerlund left a comment

Choose a reason for hiding this comment

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

  • Fix is valid. Many thanks for catching that. 👍🏼
  • (This if condition is a menace in the first place and probably shouldn't be here. If the SDK thinks we need to run the update func at this point, it should build the update opts struct unconditionally.)
  • As previous reviewers noted, this needs a test. It sounds like you're done here, so I'm going to replace this PR once I get a test written. No one merge this in the meantime, please.

## UNRELEASED

BUG FIXES:
* `r/tfe_policy`: Fix the provider ignoring updates to the `query` field and pretending it issued an API call when it didn't.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* `r/tfe_policy`: Fix the provider ignoring updates to the `query` field and pretending it issued an API call when it didn't.
* `r/tfe_policy`: Fix the provider ignoring updates to the `query` field.

Valid to be grouchy about an aggravating bug 👍🏼, but I think we can leave "ignores it -> definition of ignoring it" as an exercise for the reader.

@brandonc brandonc mentioned this pull request Nov 27, 2023
@brandonc
Copy link
Collaborator

Moved your commits to #1154 and unskipped the resource integration tests

@brandonc brandonc closed this Nov 27, 2023
opentofu-provider-sync-service-account pushed a commit to opentofu/terraform-provider-tfe that referenced this pull request Nov 28, 2023
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.

5 participants