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

core: Add resource IDs to apply-errors + prevent error duplication #2815

Merged

Conversation

radeksimko
Copy link
Member

This is a proposal for fixing #2801 and similar with a more systematic approach.

Current state:

aws_route53_record.www: Creating...
  fqdn:               "" => "<computed>"
  name:               "" => "www.notexample.com"
  records.#:          "" => "1"
  records.3619153832: "" => "127.0.0.1"
  ttl:                "" => "300"
  type:               "" => "A"
  zone_id:            "" => "WRONGID"
aws_route53_record.www: Error: 1 error(s) occurred:

* NoSuchHostedZone: No hosted zone found with ID: WRONGID
    status code: 404, request id: [d7febede-3063-11e5-9472-2971854b3f91]
Error applying plan:

1 error(s) occurred:

* NoSuchHostedZone: No hosted zone found with ID: WRONGID
    status code: 404, request id: [d7febede-3063-11e5-9472-2971854b3f91]

Terraform does not automatically rollback in the face of errors.
Instead, your Terraform state file has been partially updated with
any resources that successfully completed. Please address the error
above and apply again to incrementally change your infrastructure.

this PR is changing it into the following (no duplicate errors and resource ID mentioned):

aws_route53_record.www: Creating...
  fqdn:               "" => "<computed>"
  name:               "" => "www.notexample.com"
  records.#:          "" => "1"
  records.3619153832: "" => "127.0.0.1"
  ttl:                "" => "300"
  type:               "" => "A"
  zone_id:            "" => "WRONGID"
Error applying plan:

1 error(s) occurred:

* aws_route53_record.www: NoSuchHostedZone: No hosted zone found with ID: WRONGID
    status code: 404, request id: [1db1c357-3064-11e5-8c8a-916e2ed8af08]

Terraform does not automatically rollback in the face of errors.
Instead, your Terraform state file has been partially updated with
any resources that successfully completed. Please address the error
above and apply again to incrementally change your infrastructure

Here's a simple way to trigger an error (and see the difference):

provider "aws" {
    region = "us-west-2"
}

resource "aws_route53_record" "www" {
   zone_id = "WRONGID"
   name = "www.notexample.com"
   type = "A"
   ttl = "300"
   records = ["127.0.0.1"]
}

git blame command/hook_ui.go only brought up @mitchellh , so I think he would be the best candidate for reviewing.

The other option would be to keep the error printing in UiHook which has the advantage of more "real-time" feedback, but in that case we should:

  1. Print it in red (it is now bold white)
  2. Somehow filter these errors out in eval_apply.go so it's not duplicated.

FYI @failshell @catsby

@radeksimko
Copy link
Member Author

In fact it's not fixing #2801 (yet) since it's just addressing apply, not refresh, I'll think about refresh too. Maybe similar approach can be taken.

@radeksimko
Copy link
Member Author

Ok, same approach for refresh applied, now I need your votes/opinion about reporting via UiHook or aggregating errors and printing at the end.

I'm slightly more inclined to hooks, but both solutions have different pros/cons.

@radeksimko radeksimko mentioned this pull request Jul 22, 2015
@catsby
Copy link
Contributor

catsby commented Jul 24, 2015

I only tested this with the Route 53 example from 2801, but overall 👍 to this idea. The change seems small enough, but word from @mitchellh or @phinze may be prudent

@phinze
Copy link
Contributor

phinze commented Jul 27, 2015

This is nice! LGTM

@radeksimko
Copy link
Member Author

So you both prefer error aggregation over real-time errors then?
Just checking you did not overlook my question... :)

@phinze
Copy link
Contributor

phinze commented Jul 27, 2015

Ah thanks for pointing that out - I had overlooked the impl question.

I think getting resource IDs into the error message is an important enough win that it's better to land this as-is and we can always circle back if we find that waiting for the errors to output becomes annoying. 👍

@radeksimko
Copy link
Member Author

@phinze Agreed, it can be revisited later.

@ghost
Copy link

ghost commented May 1, 2020

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@ghost ghost locked and limited conversation to collaborators May 1, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants