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 Disconnect references from Shavar services #111

Open
1 of 3 tasks
say-yawn opened this issue Feb 3, 2020 · 7 comments
Open
1 of 3 tasks

Remove Disconnect references from Shavar services #111

say-yawn opened this issue Feb 3, 2020 · 7 comments

Comments

@say-yawn
Copy link
Contributor

say-yawn commented Feb 3, 2020

About this Issue

After merging the changes from #110 I saw that the staging S3 showed that there were files updated when we expected to updates:
Screen Shot 2020-02-03 at 1 23 57 PM
Diffs of the logs before and after merging #110 shows that the order of the domains have changed causing the list to be "changed" (look at the diff between the .log files attached).
ads-track-digest256-BEFORE.log
ads-track-digest256.log
We should wait to apply #110 to versioned branches so that the domains are added alphabetically to prevent not-real update like this one. We should also remove all the Disconnect category references in the services and configs.

Acceptance Criteria

@boolean5
Copy link
Contributor

Hi @skim1102, in continuation of mozilla-services/shavar-list-creation#121, I was thinking about sending a PR that removes references to the Disconnect category and to disconnect_mapping.json from the shavar-prod-lists repository. This would address the 3rd A/C of this issue. Should I go ahead or have you started working on this already?

@say-yawn
Copy link
Contributor Author

say-yawn commented Apr 2, 2020

@boolean5 I went ahead and added the issue from shavar-list-creation repo in the description of the issue. Let's wait to get all the acceptance criteria fulfilled before we check the third acceptance criteria. Alternatively, the second criteria:

The domains added in the list should be alphabetical to prevent not-real update like this one

has not been worked on yet and would be a good extension for the work you have been doing to remove Disconnect category and JSON file references. Would you like to pick it up as an extended work you have been doing?

@boolean5
Copy link
Contributor

boolean5 commented Apr 6, 2020

Yes, working on the 2nd A/C sounds good. Thanks for suggesting this. I'll get back to you if I have any questions.

@boolean5
Copy link
Contributor

boolean5 commented Apr 9, 2020

@skim1102, some more context on this would be very helpful.

It is not clear to me how the application of #110 caused the order of domains to change. Could you please explain this a little?

Was there a domain that was added out of alphabetical order in #110? Does it have to do with the way the Disconnect list parser parses disconnect-blacklist.json?

@say-yawn
Copy link
Contributor Author

say-yawn commented Apr 20, 2020

@boolean5 certainly! I apologize for the delayed response.

It is not clear to me how the application of #110 caused the order of domains to change. Could you please explain this a little?

Yes, as you have mentioned, the Disconnect list parser uses .add as seen here which does not impose a specific order (since Set is an unordered collection). However, when the remapping of the Disconnect category domains into the respective categories happened here, you can see that the companies and their domains were added in alphanumerical order. This resulted in the list being "changed" even though the contents/domains in the list were not changed. While we can't enforce the ordering in trackingprotection-tools package, we can do so in the shavar-list-creation repo and prevent a list from being "updated" due to changes in the order of the domains written to the file/list.

@say-yawn
Copy link
Contributor Author

Also, if you intend to keep working on this issue, please assign yourself to mozilla-services/shavar-list-creation#132. Thank you!

@boolean5
Copy link
Contributor

Thanks for explaining this!

I can't self-assign mozilla-services/shavar-list-creation#132 because I don't have write permissions for that repository. I still intend to work on it though, so feel free to assign it to me.

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

No branches or pull requests

2 participants