-
-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
support alternative raw without 0.0.0.0 #2756
base: master
Are you sure you want to change the base?
Conversation
Thank you for submitting this pull request! We’ll get back to you as soon as we can! |
updateHostsFile.py
Outdated
@@ -259,6 +259,10 @@ def main(): | |||
source_data_filename = settings["sourcedatafilename"] | |||
no_unified_hosts = settings["nounifiedhosts"] | |||
|
|||
settings["targetip"] = ( | |||
None if settings["targetip"] == "None" else settings["targetip"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I preferred to go with an explicit None
instead of ""
Hey @ldlac thank you for this! Looks cool! Looks thisclose 🤏🏻 to mergable, right out of the box, which is awesome 👍🏻 Can I please ask for two additional things?
Good work! |
Good recommendation for the I added an insightful explanation in the readme, let me know what you think |
By doing so, we'll break ipv6 no? |
@ldlac good point. |
I changed it to |
@ldlac thank you very much! All tests pass, and all this behaves exactly as expected. Except for one thing 😄 With the Observe: we still get that header section with Expected: (well, I would expect...) no such header, since this is not a hosts file now. So maybe this PR should also bracket the code that writes the header section. I also now wonder... should the I'm willing to be convinced otherwise in any and all these things. Welcome to Open Source 🙃 The first 50 lines of the (misnamed)
|
@ldlac an idea: we already have internal support for some of what I suggest above ⬆️ maybe the
|
@@ -287,19 +302,22 @@ def main(): | |||
merge_file = create_initial_file( | |||
nounifiedhosts=no_unified_hosts, | |||
) | |||
remove_old_hosts_file(settings["outputpath"], "hosts", settings["backup"]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I made few changes here since I introduced options["outputfilename"]
@@ -943,14 +966,6 @@ def remove_dups_and_excl(merge_file, exclusion_regexes, output_file=None): | |||
if line and not line.startswith("#"): | |||
settings["exclusions"].append(line) | |||
|
|||
if not os.path.exists(settings["outputpath"]): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I changed output_file to required, and I made a few changes here, the file is already ensure created by remove_old_hosts_file
@@ -906,16 +928,18 @@ def minimise_file(input_file, target_ip, output_file): | |||
for line in input_file.readlines(): | |||
line = line.decode("UTF-8") | |||
|
|||
if line.startswith(target_ip): | |||
lines.append(line[: line.find("#")].strip() + "\n") | |||
if target_ip is None or line.startswith(target_ip): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it was desired, there was some empty \n
with the minimised
flag, this will fix those
Thanks for the feedback, I added 3 comments to support my changes to support the Let me know if you think we should modify the header, I think it's fine even it if it mentions There's a few things that still bother me, the blacklist, the custom host file, those could still be pass as arg with the |
Support this request #2719
how it should be used
python updateHostsFile.py -a -g -i None
I made it that it cannot be combined with -c or -m, should it be?