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 ignore_errors argument to data sources #76

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

sergei-ivanov
Copy link
Contributor

Fixes #75

@bodgit
Copy link
Contributor

bodgit commented Feb 13, 2019

Personally, I'd rather see this option added to all of the datasources rather than only add it to some of them, otherwise they become inconsistent with each other.

Also, (and I'm not sure if this is possible), you are potentially masking any type of DNS error behind this boolean rather than specifically just NXDOMAIN.

@sergei-ivanov
Copy link
Contributor Author

I was about to do it for all data sources, but had to stop myself, because I wanted to hear some feedback on the proposed changes (much appreciated!). I'll be happy to introduce the same logic for all remaining record types.

I have also spent some time looking at the underlying library implementation, but I could not figure out a clean and reliable way to distinguish "no such host" failures from other errors. I looked at it earlier, and I looked at it again just now. Still cannot figure it out.

So may be in this context ignore_missing is a slightly misleading name, and I'll appreciate ideas here. I thought at some point to invert that flag and call it fail_on_error, with true being the default (to preserve the current behaviour).

@sergei-ivanov
Copy link
Contributor Author

Sorry, it took a few months to get back to this. I have rebased on the latest master, renamed the argument to ignore_errors and added it to all data sources. Documentation has been updated accordingly.

@sergei-ivanov sergei-ivanov changed the title Introduce ignore_missing argument to A, AAAA and CNAME data Introduce ignore_errors argument to data sources Jun 17, 2019
@sergei-ivanov
Copy link
Contributor Author

I'll fix the failing test tonight.

@sergei-ivanov
Copy link
Contributor Author

Looks like the DNS PTR record has been updated on Google side:

$ dig -x 8.8.8.8 -t PTR

;; ANSWER SECTION:
8.8.8.8.in-addr.arpa.	68365	IN	PTR	dns.google.

# Conflicts:
#	dns/data_dns_ptr_record_set_test.go
@ghost ghost added the documentation label Jul 22, 2019
# Conflicts:
#	CHANGELOG.md
#	dns/data_dns_cname_record_set_test.go
#	dns/data_dns_ptr_record_set_test.go
@ghost ghost added size/XL and removed size/L labels Jul 28, 2019
@hashicorp-cla
Copy link

hashicorp-cla commented Nov 22, 2020

CLA assistant check

Thank you for your submission! We require that all contributors sign our Contributor License Agreement ("CLA") before we can accept the contribution. Read and sign the agreement

Learn more about why HashiCorp requires a CLA and what the CLA includes


1 out of 2 committers have signed the CLA.

  • sergei-ivanov
  • Pam Selle

Pam Selle seems not to be a GitHub user.
You need a GitHub account to be able to sign the CLA. If you already have a GitHub account, please add the email address used for this commit to your account.

Have you signed the CLA already but the status is still pending? Recheck it.

Base automatically changed from master to main February 1, 2021 17:27
@freakinhippie
Copy link

This seems silly. The hashicorp-cla bot wants the CLA signed by a user that merged master into the feature branch. Perhaps it should have been a rebase rather than a merge but it doesn't make much sense to require the CLA for that.

I any case, this feature would be very helpful for me. I'd be willing to put some effort into getting this up-to-date if that's acceptable.

@sergei-ivanov
Copy link
Contributor Author

The provider has moved on a lot since the original PR and especially the tests were relocated and substantially refactored. I started rebasing, but it quickly became a major chore, and I had to put it on hold because of a lack of spare time on my part. I'll try to get back to it, but I need some encouragement from the provider team, because I don't want to waste my effort on something that might be ignored.

@atrull
Copy link

atrull commented Nov 19, 2023

I'd like this one too @sergei-ivanov #sergeistandard <B see my overbuilt myip module, where I want more survivability on silly things.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Optionally ignore DNS resolution errors in data sources
6 participants