-
Notifications
You must be signed in to change notification settings - Fork 7
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
Unify Configuration Management #990
Conversation
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.
The code looks good! I still need to do more testing, and read BSK, but I’m already leaving the comments that I have. Thanks @jaceklyp!
case .privacyConfiguration: return URL(string: "https://staticcdn.duckduckgo.com/trackerblocking/config/v2/macos-config.json")! | ||
case .surrogates: return URL(string: "https://duckduckgo.com/contentblocking.js?l=surrogates")! | ||
case .trackerDataSet: return URL(string: "https://staticcdn.duckduckgo.com/trackerblocking/v3/apple-tds.json")! | ||
// In archived repo, to be refactored shortly (https://staticcdn.duckduckgo.com/useragents/social_ctp_configuration.json) |
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.
Is this comment still relevant? If so, can you make it more clear? (I have no idea what is it about)
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.
@@ -24,7 +24,7 @@ public struct UserDefaultsWrapper<T> { | |||
public enum Key: String, CaseIterable { | |||
|
|||
case configLastUpdated = "config.last.updated" | |||
case configStorageTrackerRadarEtag = "config.storage.trackerradar.etag" | |||
case configStorageTrackerRadarEtag = "config.storage.trackerDataSet.etag" |
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.
Won't we need some migration code to recognize an etag stored under the old name?
#endif | ||
|
||
Pixel.fire(.debug(event: .trackerDataCouldNotBeLoaded, error: error)) |
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.
That is getting fired on a fresh installation, I don't think it's intended.
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.
This works great for me 👍 I’ve tested configuration fetching, ATB requests and feedback sending. Also verified that configuration fetching correctly happens in background queues. Great job @jaceklyp 💪
Task/Issue URL: https://app.asana.com/0/72649045549333/1203797041821336/f iOS PR: duckduckgo/iOS#1512 macOS PR: duckduckgo/macos-browser#990 Description: Use common API and Configuration modules located in BSK for fetching privacy related assets. Add payload validation.
Task/Issue URL: https://app.asana.com/0/72649045549333/1203797041821336/f
Tech Design URL:
CC: @bwaresiak
Description:
Use common API and Configuration modules located in BSK for fetching privacy related assets.
Add payload validation.
Steps to test this PR:
Internal references:
Software Engineering Expectations
Technical Design Template
When ready for review, remember to post the PR in MM