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

Introduce tfexec.ErrStateLocked to represent locked state error #221

Merged
merged 7 commits into from
Oct 4, 2021

Conversation

alec-rabold
Copy link
Contributor

@alec-rabold alec-rabold commented Sep 26, 2021

Adds parsing for errors where Terraform fails to acquire a state lock. Returns a first-class ErrStateLocked error, including the corresponding LockInfo.

Uses a slightly different LockInfo structure from upstream Terraform:

  • Excludes the Info section simply out of difficulty parsing out where that blurb ends
  • Treats Created as a string instead of date, mostly because I wasn't sure how to handle the (unlikely) error from time.Parse() in wrapExitError()

Side note: I don't see as much of a use-case for an Unlock() error, but I'm happy to add that in as well.

I've created a mini test-case here: https://play.golang.org/p/aAeRS5yOb7x. I can add some test cases for exit_errors.go if that's preferred though.

(Related to #222)

@hashicorp-cla
Copy link

hashicorp-cla commented Sep 26, 2021

CLA assistant check
All committers have signed the CLA.

@radeksimko radeksimko added the enhancement New feature or request label Sep 27, 2021
Copy link
Member

@radeksimko radeksimko left a comment

Choose a reason for hiding this comment

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

This looks like a useful addition, thank you for the PR.

Treats Created as a string instead of date, mostly because I wasn't sure how to handle the (unlikely) error from time.Parse() in wrapExitError()

I think that could be handled the same way as if the error message doesn't match the expression, i.e. just fall through to the generic error.

As for tests - I think it would be nice to have them, but we don't seem to have any unit tests directly testing any of these expressions. I am assuming it's probably because the risk of the expression being wrong here is acceptable - we just fall back to a more generic error.

@kmoe correct me if I'm wrong here about my assumptions about testing - do we want to introduce tests here? I guess the bigger question is whether we treat this as a "compatibility guarantee" in any way as we head towards v1. I'm leaning more towards no as building guarantees on top of scrapped text seems hard 😄

@alec-rabold
Copy link
Contributor Author

Thanks for taking a look @radeksimko!

I stumbled upon the terraform-exec/tfexec/internal/e2etest/errors_test.go file which looked like a good place to add a test for this as there's already a few in there.

Regarding the date-parsing, it seems the string format isn't 100% consistent after running it through the existing test suite. I'm thinking it may be best to leave that logic out.

Let me know if you folks have any thoughts or concerns there.

Copy link
Member

@radeksimko radeksimko left a comment

Choose a reason for hiding this comment

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

Thank you for that added test.

This LGTM. I just left a minor suggestion in-line which I'm going to apply, to get it merged now.

tfexec/exit_errors.go Outdated Show resolved Hide resolved
@radeksimko
Copy link
Member

The test failure is unrelated to this PR.

@radeksimko radeksimko merged commit a0da02b into hashicorp:main Oct 4, 2021
@radeksimko radeksimko changed the title add error wrapping for locked state Introduce tfexec.ErrStateLocked to represent locked state error Oct 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants