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 dig lookup argument fail_on_error #4973

Merged
merged 1 commit into from
Jul 24, 2022

Conversation

tumbl3w33d
Copy link
Contributor

with default False for backwards compatibility.

Allows fail-fast behavior on lookup failures instead of returning strings and continuing.

SUMMARY

I would like the dig lookup to offer the option to fail on lookup errors. Right now it just returns empty strings or something like 'NXDOMAIN' as string which is ugly because you notice quite late that something is wrong with your infrastructure because a dig lookup in some config template produced an unexpected result although your playbook run was clean.

ISSUE TYPE
  • Feature Pull Request
COMPONENT NAME

dig lookup plugin

ADDITIONAL INFORMATION

IMHO this should be the default, but for backwards compatibility I've disabled it.

@ansibullbot
Copy link
Collaborator

cc @jpmens
click here for bot help

@ansibullbot ansibullbot added feature This issue/PR relates to a feature request lookup lookup plugin needs_ci This PR requires CI testing to be performed. Please close and re-open this PR to trigger CI needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR new_contributor Help guide this first time contributor plugins plugin (any type) labels Jul 22, 2022
@ansibullbot ansibullbot removed the needs_ci This PR requires CI testing to be performed. Please close and re-open this PR to trigger CI label Jul 22, 2022
@jpmens
Copy link
Contributor

jpmens commented Jul 22, 2022

For the record, this behaviour was originally proposed in ansible/ansible#41192 and in ansible/ansible#41195 (at least that's the way I recall it).

I am all for this change, and think we should, indeed, make fail_on_error default to True at some stage. (Why not introduce a deprecation on the current behaviour?)

@jpmens
Copy link
Contributor

jpmens commented Jul 22, 2022

shipit

@ansibullbot ansibullbot removed the needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR label Jul 22, 2022
@github-actions
Copy link

github-actions bot commented Jul 22, 2022

Docs Build 📝

Thank you for contribution!✨

This PR has been merged and your docs changes will be incorporated when they are next published.

Copy link
Collaborator

@felixfontein felixfontein 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! Could you please add a changelog fragment? Thanks :)

@felixfontein felixfontein added check-before-release PR will be looked at again shortly before release and merged if possible. backport-5 labels Jul 23, 2022
@felixfontein
Copy link
Collaborator

I am all for this change, and think we should, indeed, make fail_on_error default to True at some stage. (Why not introduce a deprecation on the current behaviour?)

Since the current behavior has been the default for a very long time, I would wait a bit before making it configurable and deprecating the old behavior, to give users at least some versions of this collection that are backwards compatible but do not yet spit out deprecation warnings.

(I'm fine with announcing deprecation in the next major release, i.e. 6.0.0, and announcing the default value change for version 8.0.0. In fact the deprecation can already be merged in a follow-up, since by keeping it in main and not backporting it will be only in 6.x.y releases, but not in 5.x.y releases.)

with default False for backwards compatibility.

Allows fail-fast behavior on lookup failures instead of returning strings and continuing.
@ansibullbot ansibullbot added needs_ci This PR requires CI testing to be performed. Please close and re-open this PR to trigger CI and removed needs_ci This PR requires CI testing to be performed. Please close and re-open this PR to trigger CI labels Jul 23, 2022
@felixfontein felixfontein removed the check-before-release PR will be looked at again shortly before release and merged if possible. label Jul 24, 2022
@felixfontein felixfontein merged commit 2662bc8 into ansible-collections:main Jul 24, 2022
This was referenced Nov 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature This issue/PR relates to a feature request has_issue lookup lookup plugin new_contributor Help guide this first time contributor plugins plugin (any type)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants