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

acceptance tests - update plan check logic to not error if the resource isn't found in the plan #28640

Merged
merged 1 commit into from
Feb 6, 2025

Conversation

stephybun
Copy link
Member

Community Note

  • Please vote on this PR by adding a 👍 reaction to the original PR to help the community and maintainers prioritize for review
  • Please do not leave comments along the lines of "+1", "me too" or "any updates", they generate extra noise for PR followers and do not help prioritize for review

Description

The plan check CheckPlan was introduced by #27319 to enable the acceptance testing shim to catch and error out from update tests that triggered a ForceNew. The current logic in CheckPlancauses a number of acceptance tests in the provider to fail with an error similar to the one below:

------- Stdout: -------
=== RUN   TestAccImage_standaloneImage
=== PAUSE TestAccImage_standaloneImage
=== CONT  TestAccImage_standaloneImage
    testcase.go:173: Step 1/4 error: Pre-apply plan check(s) failed:
        azurerm_image.test - Resource not found in plan ResourceChanges
--- FAIL: TestAccImage_standaloneImage (18.69s)
FAIL

Tests failing with this error rely on a setup configuration as the first test step before applying the test configuration that's pertinent for testing the actual resource.

This PR inverts the logic in CheckPlan to only check for a Replace action and error if the resource exists in the plan as opposed to error'ing if it's missing.

Testing

Ideally the entire testing suite is run to catch any edge cases, given the size and scope of that it's not a feasible undertaking.

I ran three tests to validate the behaviour is as expected:

TestAccImage_standaloneImage - previously failed, now passes
TestAccAppConfigurationKey_basicNoValue - failed because it triggers a ForceNew, still fails meaning the original intent of the check is still intact
TestAccAppConfigurationKey_KVToVault - passed, still passes

@stephybun stephybun requested a review from a team as a code owner January 30, 2025 13:40
@stephybun stephybun force-pushed the b/update-plan-check-logic branch from 92c04c0 to bf47a29 Compare January 30, 2025 13:40
Copy link
Member

@jackofallops jackofallops left a comment

Choose a reason for hiding this comment

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

Thanks @stephybun - This LGTM 🍳

@stephybun stephybun merged commit af5bef1 into main Feb 6, 2025
29 checks passed
@stephybun stephybun deleted the b/update-plan-check-logic branch February 6, 2025 07:25
@github-actions github-actions bot added this to the v4.18.0 milestone Feb 6, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants