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

config: fix for #786 - better error reporting #818

Closed
wants to merge 2 commits into from
Closed

config: fix for #786 - better error reporting #818

wants to merge 2 commits into from

Conversation

publysher
Copy link

As discussed in #786 , terraform crashes when no providers could be found. I can think of no good reason to run terraform without any provider or provisioner, so I've added extra error reporting.

@@ -105,6 +106,13 @@ func (c *Config) Discover() error {
}
}

if len(c.Providers) == 0 {
return errors.New("no providers found")
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks! Just a cosmetic note about the style.... I think it's more idiomatic (and inline with the code style used in TF) to write this as return fmt.Errorf("no providers found")

Copy link
Author

Choose a reason for hiding this comment

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

You're right. I saw some errors.New calls, but fmt.Errorf is clearly the dominant style.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, your right about that... I also see some errors.New in the code (now that I'm looking for it 😉), so apparently it doesn't matter that much and isn't used consistently...

Thanks!

Following dominant style in TF, as suggested by svanharmelen.
@mitchellh
Copy link
Contributor

Thanks! This is one way to fix it, but I think this should actually happen later during config validation. It should be an error if we can't find any provider (not just have 0) that we intend to use. Let me see if I can work with #786 to make that happen.

@mitchellh
Copy link
Contributor

Yeah, as I suspected the root cause was actually pretty deep in TF. I caught that. Thanks!

@mitchellh mitchellh closed this Jan 16, 2015
@ghost
Copy link

ghost commented May 4, 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 4, 2020
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.

3 participants