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 more helpful error message for misconfiguration in profiles.yml #2627

Merged
merged 4 commits into from
Jul 17, 2020

Conversation

kning
Copy link
Contributor

@kning kning commented Jul 15, 2020

resolves #2569

Description

Adds an additional catch for if the supplied target returns no data and returns a more helpful error message.

Checklist

  • I have signed the CLA
  • I have run this code in development and it appears to resolve the stated issue
  • This PR includes tests, or tests are not required/relevant for this PR
  • I have updated the CHANGELOG.md and added information about my change to the "dbt next" section. <-- not sure if this change warrants a mention quite yet but let me know if you'd like me to add.

Further Validation

Additional screenshot of what the new terminal output looks like (tested against the example target referenced in the issue):
image

@cla-bot cla-bot bot added the cla:yes label Jul 15, 2020
Copy link
Contributor

@beckjake beckjake left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution @kning!

I have a minor copy suggestion I'd like to get feedback on from Jeremy, and a suggestion on catching more bad profiles.

Can you also add an integration test? I think the appropriate place is in test/integration/049_debug_test/test_debug.py.

You can update the postgres profile to add a new entry with "none_target": None instead of an actual target definition, and then add a new test that runs debug --target none_target and make sure it says something reasonable.

Finally, please make a CHANGELOG.md update following the existing pattern, so we can give you credit in our next release! Add yourself to the contributors list for the upcoming release, and add a summary under Features to this PR and the issue. The name of this PR should be a fine summary.

@@ -197,6 +197,11 @@ def _get_profile_data(
.format(profile_name, target_name, outputs))
raise DbtProfileError(msg, result_type='invalid_target')
profile_data = outputs[target_name]

if profile_data is None:
msg = (f"{target_name} is misconfigured in your profiles.yml")
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
msg = (f"{target_name} is misconfigured in your profiles.yml")
msg = (
f"output '{target_name}' of profile '{profile_name}' is "
f"misconfigured in profiles.yml"
)

It'd be nice to point it at where the issue is a bit more clearly in large profiles, here's one way we could add the profile name in and show that we're talking about outputs: values.

@jtcohen6, do you have any other suggestions on the copy?

@@ -197,6 +197,11 @@ def _get_profile_data(
.format(profile_name, target_name, outputs))
raise DbtProfileError(msg, result_type='invalid_target')
profile_data = outputs[target_name]

if profile_data is None:
Copy link
Contributor

Choose a reason for hiding this comment

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

It'd be good to check if not isinstance(profile_data, dict) instead - if it's anything but a dict, we know it's misconfigured.

@kning
Copy link
Contributor Author

kning commented Jul 16, 2020

thanks taking a look

@kning
Copy link
Contributor Author

kning commented Jul 16, 2020

A question on the integration test @beckjake.

dbt debug actually outputs fine as long as there is one correctly configured target. Thus, adding a new target definition to the existing postgres profile doesn't actually trigger the test case.

I think I'd need to create a separate postgres profile with only an empty target. However, I'm not sure the best way to do this. The way the test is set up it looks like you can only have one postgres profile?

@beckjake
Copy link
Contributor

dbt debug actually outputs fine as long as there is one correctly configured target. Thus, adding a new target definition to the existing postgres profile doesn't actually trigger the test case.

Really? I think it'd be sufficient to specify the bad target on the command line with --target. For example, in test_postgres_wronguser, we run self.run_dbt(['debug', '--target', 'wronguser']). You could do something similar with an empty target, right?

@kning
Copy link
Contributor Author

kning commented Jul 17, 2020

ready for review now @beckjake

Copy link
Contributor

@beckjake beckjake 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 good to me! Thanks for your contribution to dbt, and thanks for the test!

I'll merge this once the test passes. Windows tests might fail because of a config issue, that's fine - this is not OS-specific behavior, and I have a fix for that in a different open PR.

CHANGELOG.md Outdated
@@ -6,13 +6,15 @@
### Features
- Added support for Snowflake query tags at the connection and model level ([#1030](https:/fishtown-analytics/dbt/issues/1030), [#2555](https:/fishtown-analytics/dbt/pull/2555/))
- Added option to specify profile when connecting to Redshift via IAM ([#2437](https:/fishtown-analytics/dbt/issues/2437), [#2581](https:/fishtown-analytics/dbt/pull/2581))
- Add more helpful error message for misconfiguration in profiles.yml ([#2627](https:/fishtown-analytics/dbt/issues/2569))
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
- Add more helpful error message for misconfiguration in profiles.yml ([#2627](https:/fishtown-analytics/dbt/issues/2569))
- Add more helpful error message for misconfiguration in profiles.yml ([#2569](https:/fishtown-analytics/dbt/issues/2569), [#2627](https:/fishtown-analytics/dbt/pull/2627))

CHANGELOG.md Outdated

### Fixes
- Adapter plugins can once again override plugins defined in core ([#2548](https:/fishtown-analytics/dbt/issues/2548), [#2590](https:/fishtown-analytics/dbt/pull/2590))

Contributors:
- [@brunomurino](https:/brunomurino) ([#2437](https:/fishtown-analytics/dbt/pull/2581))
- [@DrMcTaco](https:/DrMcTaco) ([#1030](https:/fishtown-analytics/dbt/issues/1030)),[#2555](https:/fishtown-analytics/dbt/pull/2555/))
- [@kning](https:/kning) ([#2627](https:/fishtown-analytics/dbt/issues/2569))
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
- [@kning](https:/kning) ([#2627](https:/fishtown-analytics/dbt/issues/2569))
- [@kning](https:/kning) ([#2627](https:/fishtown-analytics/dbt/pull/2627))

@beckjake beckjake merged commit 7f0c1f8 into dbt-labs:dev/marian-anderson Jul 17, 2020
@beckjake
Copy link
Contributor

All looks great, this will release in 0.18.0 🎉

@kning
Copy link
Contributor Author

kning commented Jul 17, 2020

thanks @beckjake !

@kning kning deleted the ISSUE-2569_bad_target branch July 17, 2020 17:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Empty target in profiles.yml results in an unhelpful error message
2 participants