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 references to Disconnect category and disconnect_mapping.json #121

Merged

Conversation

boolean5
Copy link
Contributor

This PR closes #118 by removing all references to the Disconnect category and to disconnect_mapping.json.

@boolean5
Copy link
Contributor Author

boolean5 commented Mar 10, 2020

@skim1102, @englehardt, should I also remove the disconnect_mapping.json file itself or is it still
used by other services?

Also, I noticed that the blocklists in https://github.com/mozilla-services/shavar-experiments are still using the Disconnect category. As a result, the corresponding created lists (fastblock1-track-digest256, fastblock2-track-digest256 and fastblock3-track-digest256) now contain fewer domains than before. Should we wait for the fastblock blocklists to be updated before merging this PR?

@say-yawn
Copy link
Contributor

say-yawn commented Mar 12, 2020

Also, I noticed that the blocklists in https://github.com/mozilla-services/shavar-experiments are still using the Disconnect category. As a result, the corresponding created lists (fastblock1-track-digest256, fastblock2-track-digest256 and fastblock3-track-digest256) now contain fewer domains than before. Should we wait for the fastblock blocklists to be updated before merging this PR?

@englehardt, as @boolean5 mentioned, the fastblock lists do indeed have Disconnect in its json files, see this fastblock1.json example. It seems the last time shavar-experiments was updated 2 years ago (1yr ago for CODE_OF_CONDUCT). Can we remove the fastblock from shavar-list-creation? Or should we update the shavar-experiments to no longer use the Disconnect category like we did on shavar-prod-lists?

@say-yawn
Copy link
Contributor

@skim1102, @englehardt, should I also remove the disconnect_mapping.json file itself or is it still
used by other services?

@boolean5, we should keep the disconnect-mapping.json for now to ensure other components (iOS or Android) no longer uses/references the file. I will update the issue once I find out.

@englehardt
Copy link
Contributor

@englehardt, as @boolean5 mentioned, the fastblock lists do indeed have Disconnect in its json files, see this fastblock1.json example. It seems the last time shavar-experiments was updated 2 years ago (1yr ago for CODE_OF_CONDUCT). Can we remove the fastblock from shavar-list-creation? Or should we update the shavar-experiments to no longer use the Disconnect category like we did on shavar-prod-lists?

Yes we can remove the Fastblock lists.

@boolean5
Copy link
Contributor Author

Yes we can remove the Fastblock lists.

I added a commit that removes them. Thanks for clarifying this. Should we also remove the Fastblock lists from https://github.com/mozilla-services/shavar-list-creation-config/ before merging this?

@say-yawn
Copy link
Contributor

@boolean5, yes. Along with that in config repo for shavar-list-creation we should also remove all refernces to Disconnect category in Stage and Prod .ini files.

@boolean5
Copy link
Contributor Author

After discussing this with @skim1102, I'm pushing a commit that removes the disconnect_mapping.json file.

@boolean5
Copy link
Contributor Author

@boolean5, yes. Along with that in config repo for shavar-list-creation we should also remove all refernces to Disconnect category in Stage and Prod .ini files.

I just submitted a PR for this: mozilla-services/shavar-list-creation-config#60

@boolean5
Copy link
Contributor Author

boolean5 commented Mar 17, 2020

While working on this I noticed that the deprecated Disconnect category is still present in https://github.com/mozilla-services/shavar-staging-lists/blob/master/disconnect-blacklist.json and https://github.com/mozilla-services/shavar-test-lists/blob/master/track.json. These lists are probably not actively used right now but I thought I'd mention it here in case they need to be updated.

@say-yawn
Copy link
Contributor

While working on this I noticed that the deprecated Disconnect category is still present in https://github.com/mozilla-services/shavar-staging-lists/blob/master/disconnect-blacklist.json and https://github.com/mozilla-services/shavar-test-lists/blob/master/track.json. These lists are probably not actively used right now but I thought I'd mention it here in case they need to be updated.

@boolean5, thanks for bringing these up. Can you create an issue on shavar-list-creation to start documenting whether the repositories are being used or referenced anywhere?

Copy link
Contributor

@say-yawn say-yawn left a comment

Choose a reason for hiding this comment

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

Test on local dev and works as expected.

@say-yawn
Copy link
Contributor

@boolean5, from my previous experiences I do not merge after 4:30PM my local time to save myself from the potential issues caused by the "5PM push". As a result, I will merge this first thing tomorrow!

@say-yawn
Copy link
Contributor

Screen Shot 2020-03-19 at 4 42 47 PM

Shucks, I cannot merge this unless all the commits are signed. Can you sign the commits?

Remove all references to the Disconnect category and to
disconnect_mapping.json, since the Disconnect category has been removed.
Remove the disconnect_mapping.json file, as it is not used anymore.
@boolean5 boolean5 force-pushed the remove-disconnect-references branch from 9296e00 to fa15cdc Compare March 20, 2020 13:28
@boolean5
Copy link
Contributor Author

Sure, I signed the commits and rebased them on the current master.

@boolean5
Copy link
Contributor Author

Can you create an issue on shavar-list-creation to start documenting whether the repositories are being used or referenced anywhere?

Done! I opened the issue here: #126

@boolean5
Copy link
Contributor Author

I noticed that category_domains.py in mozilla-services/shavar-prod-lists#156 is planning to use disconnect_mapping.json to compare shavar-prod-lists and disconnect-tracking-protection lists. I see two options now:

  1. Removing from this PR the commit that removes the disconnect_mapping.json file.
  2. Keeping this PR as it is and moving disconnect_mapping.json to shavar-prod-lists, which is the only repo using it. This has been discussed before in Remove use of google-mapping.json mozilla-mobile/firefox-ios#5968 (comment) and seems like a good idea.

@skim1102, what do you think?

@say-yawn
Copy link
Contributor

@boolean5 since we are going to be removing that file I do not think we should move the file. I left a comment to remove references to disconnect_mapping.json

@say-yawn say-yawn merged commit bd0f384 into mozilla-services:master Apr 1, 2020
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.

Remove Disconnect and Google mapping references
3 participants