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

Add -detailed-exitcode option to Plan #55

Merged
merged 4 commits into from
Aug 25, 2020
Merged

Add -detailed-exitcode option to Plan #55

merged 4 commits into from
Aug 25, 2020

Conversation

sudomateo
Copy link
Contributor

@sudomateo sudomateo commented Aug 18, 2020

This adds the -detailed-exitcode option to tfexec.Plan() and returns
a boolean detailing whether a plan diff is empty or non-empty.

This allows the caller to make decisions when the plan has changes.

hasChanges, err := tf.Plan(context.Background())
if err != nil {
  panic(err)
}
if hasChanges {
  // Do something when the plan has changes.
}

@paultyng
Copy link
Contributor

Per the internal conversation, I think we should make the following changes to surface this functionality:

  • always pass -detailed-exitcode for Plan
  • change the Plan function signature to Plan(...) (bool, error) where the bool represents if a diff is present or not (exit code 0 or 2)

We probably need a test case for a diff and no diff, not sure if that is present in the e2e testing yet, so probably need to add a test case that modifies the TF file, I can assist with this though if you have any questions.

@paultyng paultyng added the enhancement New feature or request label Aug 18, 2020
@sudomateo
Copy link
Contributor Author

@paultyng @kmoe this is ready for review. Thank you for the feedback!

func (tf *Terraform) Plan(ctx context.Context, opts ...PlanOption) (bool, error) {
cmd := tf.planCmd(ctx, opts...)
err := tf.runTerraformCmd(cmd)
if err != nil && cmd.ProcessState.ExitCode() == 2 {
Copy link
Contributor

Choose a reason for hiding this comment

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

You probably need to confirm this is ExitError, ie something like:

Suggested change
if err != nil && cmd.ProcessState.ExitCode() == 2 {
var exitErr *exec.ExitError
if errors.As(err, &exitErr) && cmd.ProcessState.ExitCode() == 2 {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't believe this will work since the err here is returned from parseError() (hitting the default case). At that time the original err is essentially discarded.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should I perhaps add another case to parseError() that checks the original error and returns a new error type that I can then check from Plan()?

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah, i think we should probably fix the default case to just wrap the ExitError so that its compatible with errors.As/Is.

@paultyng
Copy link
Contributor

This needs a rebase but I think we just merge it as is. The refactor for the error handling won't be user facing, so can just be a future improvement if someone gets to it.

This adds the `-detailed-exitcode` option to `tfexec.Plan()` along with
a new error named `ErrExitCodeTwo` (name suggestions welcomed) that is
returned when the exit code of a given Terraform command is `2`.
Return a boolean that tells the caller whether the plan diff was empty
or non-empty.

Added and updates tests to be compatible with this change.
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.

2 participants