Skip to content

Conversation

bbasata
Copy link
Collaborator

@bbasata bbasata commented Mar 21, 2025

This PR adds a test to resolve the question: what happens if the first TestStep uses import mode?

This test passes 🍏

Notably: ImportStateVerify does not work, because there is no previous step state to verify against.

@bbasata bbasata force-pushed the import-as-first-test-step branch from b8a65c8 to 70f740d Compare March 24, 2025 12:47
@bbasata bbasata marked this pull request as ready for review March 24, 2025 12:53
@bbasata bbasata requested a review from a team as a code owner March 24, 2025 12:53
Base automatically changed from import-blocks to main March 24, 2025 14:53
@bbasata bbasata requested a review from austinvalle March 24, 2025 19:09
@bbasata
Copy link
Collaborator Author

bbasata commented Mar 24, 2025

I'm still not sure we should actually apply anything during ImportState tests.

One detail that I remembered: the only reason RefreshState worked here is ImportStatePersist. Otherwise, ImportState operates in an isolated working directory.

As mentioned above, I've removed RefreshState from the tests, so I can also remove ImportStatePersist from the tests for now.

@bbasata bbasata requested a review from a team March 25, 2025 14:30
@austinvalle austinvalle added this to the v1.13.0 milestone Mar 25, 2025
@bbasata bbasata enabled auto-merge (squash) March 26, 2025 12:46
Copy link
Member

@austinvalle austinvalle left a comment

Choose a reason for hiding this comment

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

The code LGTM, I do have a question about the naming of one of the fields

if err := runPlanChecks(ctx, t, plan, step.ImportPlanChecks.PreApply); err != nil {
return err
}

Copy link
Member

Choose a reason for hiding this comment

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

Not directly related to this PR, but I noticed that we don't immediately return after running this import/plan and we probably should? Or wrap the remaining of the logic in this function in the following else block.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

✅ Noted for a follow-up PR -- it will be easier to see the current picture after we merge this & the two PRs "stacked" atop it.

// TODO compare plan to state from previous step

if err := runPlanChecks(ctx, t, plan, step.ImportPlanChecks.PreApply); err != nil {
return err
Copy link
Member

Choose a reason for hiding this comment

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

nit: Should we wrap the error messages produced by the plan checks for readability?

    return fmt.Errorf("Import plan check(s) failed:\n%w", err)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

💯

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

✅ Noted for a follow-up PR -- it will be easier to see the current picture after we merge this & the two PRs "stacked" atop it.

@bbasata bbasata merged commit a23dbf2 into main Mar 27, 2025
39 checks passed
@bbasata bbasata deleted the import-as-first-test-step branch March 27, 2025 13:29
@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 9, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants