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

Remove deprecated malwaredomains list #3937

Merged
merged 1 commit into from
Dec 24, 2020
Merged

Remove deprecated malwaredomains list #3937

merged 1 commit into from
Dec 24, 2020

Conversation

yubiuser
Copy link
Member

  • I have read and understood the contributors guide, as well as this entire template.
  • I have made only one major change in my proposed changes.
  • I have commented my proposed changes within the code.
  • I have tested my proposed changes, and have included unit tests where possible.
  • I am willing to help maintain this change if there are issues with it later.
  • I give this submission freely and claim no ownership.
  • It is compatible with the EUPL 1.2 license
  • I have squashed any insignificant commits. (git rebase)

What does this PR aim to accomplish?:
Removes the deprecated malwaredomains list
For details see https://riskanalytics.com/community/ and the linked issue

Fixes #3925

Signed-off-by: Christian König <ckoenig@posteo.de>
Comment on lines 1288 to +1290
cmd=(whiptail --separate-output --checklist "Pi-hole relies on third party lists in order to block ads.\\n\\nYou can use the suggestions below, and/or add your own after installation\\n\\nTo deselect any list, use the arrow keys and spacebar" "${r}" "${c}" 5)
# In an array, show the options available (all off by default):
options=(StevenBlack "StevenBlack's Unified Hosts List" on
MalwareDom "MalwareDomains" on)
options=(StevenBlack "StevenBlack's Unified Hosts List" on)
Copy link
Member

Choose a reason for hiding this comment

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

Do we really need the whiptail select if there's only one choice?

Copy link
Member

Choose a reason for hiding this comment

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

It still gives a new user /reconfigurerer a chance to de-select the list, I guess

Copy link
Member Author

Choose a reason for hiding this comment

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

I second this.

Copy link
Member

Choose a reason for hiding this comment

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

Should make it a radio list then, if there's only one binary choice.

Copy link
Member

Choose a reason for hiding this comment

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

Have tested a radiolist, and it does not allow one to deselect if it is a single item. checklist is sufficient, I think.

@yubiuser cam you also update line 1288 to read:

cmd=(whiptail --separate-output --checklist "Pi-hole relies on third party lists in order to block ads.\\n\\nYou can use the suggestion below, and/or add your own after installation\\n\\nTo deselect the suggested list, use spacebar" "${r}" "${c}" 5)

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I was going to suggest that... but that would need to be done in a separate PR - we would need to redo some of the code further on once a selection is made.

Copy link
Member Author

Choose a reason for hiding this comment

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

How likely is it that there will be at least two adlists in the future... just asking before the code is changed here and later reverted back to the current selection menu.

Copy link
Member

Choose a reason for hiding this comment

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

That, indeed, was another hesitation I had about making the suggestion ;)

Never say never, but probably never.

Copy link
Member

@dschaper dschaper Dec 22, 2020

Choose a reason for hiding this comment

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

I've emailed the Contact Me to ask about the replacement list.

Edit: But in general I think Steven as the sole list is sufficient.

Copy link
Member Author

Choose a reason for hiding this comment

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

Usually we recommend to look at https://firebog.net/ if users ask for lists. Why don't include we some of them already? There have been ~5 lists previously - based on what have they been chosen? Can't the same requirements be used to add some of the fireborg lists?

@dschaper
Copy link
Member

The URL redirects to a page that says the list is still available for non-commercial use under a new name. Has anyone checked to see what that is or if it's available?

@yubiuser
Copy link
Member Author

I've tried to find it, but was not successful. The page states

We will continue to make a free...

But they haven't made this list easy available in the last 21 days. It might never come. I can put this on hold for...say 1 month.... and if the list pops up will change only the URL.

Copy link
Member

@PromoFaux PromoFaux left a comment

Choose a reason for hiding this comment

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

Approving to merge into dev (and therefore be reflected in the next release)

The list is dead, so no sense in having it there :)

@PromoFaux PromoFaux merged commit eb86a5e into pi-hole:development Dec 24, 2020
@yubiuser
Copy link
Member Author

Too fast. I didn't address the wording as suggested by you
#3937 (comment)

@PromoFaux
Copy link
Member

Whoops. That's fine, please make another PR with that adjustment :)

@pralor-bot
Copy link

This pull request has been mentioned on Pi-hole Userspace. There might be relevant details there:

https://discourse.pi-hole.net/t/pi-hole-core-v5-2-3-web-v5-3-and-ftl-v5-4-released/43009/1

@pralor-bot
Copy link

This pull request has been mentioned on Pi-hole Userspace. There might be relevant details there:

https://discourse.pi-hole.net/t/reinstall-issue/43070/4

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.

None yet

4 participants