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

Fix NoneType error when querying SOA records #42

Merged
merged 7 commits into from
Jul 31, 2023

Conversation

maxkrivich
Copy link
Contributor

@maxkrivich maxkrivich commented Jul 23, 2023

Hey there 👋🏻

In this PR, I have made changes to fierce function to fix NoneType error whenever SOA query return None. After applying the patch the tool will gradually shutdown and print a proper error message. I have tested the changes locally, and they work perfectly.

Example of the error:

fierce --domain belgiantrain.be

NS: ns2.belgiantrain.be. ns1.belgiantrain.be.
Traceback (most recent call last):
  File "/Users/user/Projects/fierce/.venv/bin/fierce", line 33, in <module>
    sys.exit(load_entry_point('fierce', 'console_scripts', 'fierce')())
  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/user/Projects/fierce/fierce/fierce.py", line 491, in main
    fierce(**vars(args))
  File "/Users/user/Projects/fierce/fierce/fierce.py", line 322, in fierce
    master_address = master[0].address
                     ~~~~~~^^^
TypeError: 'NoneType' object is not subscriptable

I would love to hear your feedback on these changes. Please let me know if there's anything you'd like me to modify or improve. I'm here to make any necessary adjustments to ensure this contribution aligns with the project's standards.

Cheers,
Max 👨🏻‍💻

Copy link
Owner

@mschwager mschwager left a comment

Choose a reason for hiding this comment

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

Nice find! And thank you for the fix and comprehensive description 👍

@maxkrivich
Copy link
Contributor Author

@mschwager I've fixed the error with Github actions by removing old versions of Python that are no longer available. Now it should pass the checks.

@mschwager
Copy link
Owner

@mschwager I've fixed the error with Github actions by removing old versions of Python that are no longer available. Now it should pass the checks.

Nice, thank you. If you're going to drop old Python version support in this PR, can you update these locations too:

@maxkrivich
Copy link
Contributor Author

@mschwager I've fixed the error with Github actions by removing old versions of Python that are no longer available. Now it should pass the checks.

Nice, thank you. If you're going to drop old Python version support in this PR, can you update these locations too:

* [`master`/setup.py#L41](https://github.com/mschwager/fierce/blob/master/setup.py?rgh-link-date=2023-07-29T15%3A50%3A16Z#L41)

* And add a `Removed` section: [`master`/CHANGELOG.md#unreleased](https://github.com/mschwager/fierce/blob/master/CHANGELOG.md?rgh-link-date=2023-07-29T15%3A50%3A16Z#unreleased)

Good catch. I've adjusted the PR according to the comment. Moreover, I decided to pin down the minimal required Python version to 3.8. It will reduce the maintenance burden for cases if someone still uses the deprecated version of Python.

CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
setup.py Outdated Show resolved Hide resolved
@mschwager mschwager merged commit f32f639 into mschwager:master Jul 31, 2023
@mschwager
Copy link
Owner

Thanks for fixing the tests! I dropped the 1.5.1 changes from this PR. We can add that back in when it makes sense to release a new version 👍

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

Successfully merging this pull request may close these issues.

2 participants