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.

Sign up for GitHub

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

Jump to bottom

Disconnect list update #26

Merged
merged 12 commits into from
Oct 4, 2017
Merged

Disconnect list update #26

merged 12 commits into from
Oct 4, 2017

Conversation

groovecoder
Copy link
Contributor

@groovecoder groovecoder commented Jul 10, 2017 •
edited
Loading

Builds on #23 ...

@groovecoder groovecoder requested a review from fmarier September 28, 2017 21:03
@groovecoder groovecoder force-pushed the disconnect-list-update branch from 5d8ab31 to f59a296 Compare October 3, 2017 14:15
@groovecoder groovecoder force-pushed the disconnect-list-update branch from f59a296 to 8809d6f Compare October 3, 2017 14:19
@groovecoder
Copy link
Contributor Author

@fmarier - this is ready for your r?

@groovecoder groovecoder mentioned this pull request Oct 3, 2017
Closed
fmarier
fmarier reviewed Oct 3, 2017
View reviewed changes
disconnect-entitylist.json
"aggregateintelligence.com"
]
},
"4mads": {
Copy link

Choose a reason for hiding this comment

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

We should eliminate these entries (i.e. where there is a single resource and a single property). They don't do anything because a domain is never cross-origin with itself.

It would help reduce the bloat on this list.

Copy link

Choose a reason for hiding this comment

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

By "eliminate", I mean in the script that generates the actual binary list. It's fine if these entries are in this json list.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can you file on https://github.com/mozilla-services/shavar-list-creation ?

Copy link

Choose a reason for hiding this comment

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

Filed at mozilla-services/shavar-list-creation#49

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should we block this PR on that?

Copy link

Choose a reason for hiding this comment

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

I think so because if we're not filtering those out, it will likely lead to a significant reduction in the size of the entity list.

fmarier
fmarier reviewed Oct 3, 2017
View reviewed changes
disconnect-entitylist.json Outdated
@@ -310,7 +401,8 @@
"friendfeed.com",
"instagram.com",
"fbcdn.net",
"messenger.com"
"messenger.com",
"atlassolutions.com"
Copy link

Choose a reason for hiding this comment

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

nit: whitespace problem

fmarier
fmarier reviewed Oct 3, 2017
View reviewed changes
disconnect-entitylist.json Outdated
@@ -323,7 +415,8 @@
"akamaihd.net",
"instagram.com",
"fbcdn.net",
"messenger.com"
"messenger.com",
"atlassolutions.com"
Copy link

Choose a reason for hiding this comment

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

ditto

fmarier
fmarier reviewed Oct 3, 2017
View reviewed changes
disconnect-entitylist.json Outdated
@@ -3133,6 +3275,20 @@
"gopjn.com"
]
},
"EFF": {
"properties": [
Copy link

Choose a reason for hiding this comment

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

nit:whitespace

fmarier
fmarier reviewed Oct 3, 2017
View reviewed changes
disconnect-entitylist.json Outdated
"Etarget": {
"properties": [
"etargetnet.com",
"etarget.net"
Copy link

Choose a reason for hiding this comment

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

ditto

fmarier
fmarier reviewed Oct 3, 2017
View reviewed changes
disconnect-entitylist.json Outdated
"adbureau.net",
"adecn.com",
"aquantive.com",
"advertising.microsoft.com",
Copy link

Choose a reason for hiding this comment

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

That's a redundant entry since microsoft.com is on the list already.

@groovecoder
Copy link
Contributor Author

Updated with nit-fixes.

@groovecoder
Copy link
Contributor Author

I checked that we're already doing mozilla-services/shavar-list-creation#49. @fmarier can you approve this PR and I'll merge it tomorrow so we can watch the list job run during office hours?

@groovecoder groovecoder merged commit a108745 into master Oct 4, 2017
@groovecoder groovecoder deleted the disconnect-list-update branch October 4, 2017 20:25
@groovecoder
Copy link
Contributor Author

Closing the loop here ... the new lists were generated and uploaded to S3 for shavar to serve.

relevant extract of console output from the jenkins job (link needs MozAWSDeploy access):

...
Uploaded to s3: tracking-protection-base
...
Uploaded to s3: tracking-protection-content
...
Uploaded to s3: tracking-protection-standard
Uploaded to s3: tracking-protection-full
...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fmarier fmarier fmarier approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@groovecoder @fmarier @carbureted