-
Notifications
You must be signed in to change notification settings - Fork 14
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
CPM filterlist experiment #3086
Conversation
@muodov could you please remove the local BSK and point the package to the specific branch and commit it? |
52b7718
to
4c7e831
Compare
4c7e831
to
0c77968
Compare
@@ -360,19 +360,9 @@ jobs: | |||
runs-on: ubuntu-latest | |||
|
|||
steps: | |||
- uses: actions/checkout@v4 |
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.
Can you explain these changes? Seems strange to modify the CI in main
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 CI check is verifying that the autoconsent JS bundle matches the version pinned in the dependencies. This is mainly to catch when we update package.json, but do not actually commit the recompiled JS bundle.
In this case, we're actually doing exactly that: shipping a custom version that is not released as a proper library version. I removed this check so that it doesn't fail every build while the experiment is active
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.
Understood, do we have a task scheduled for re-activating this check?
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.
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.
Fantastic thanks, then the PR LGTM if we agree that the monitoring part is enough.
You could merge BSK, release a new version (patch version bump I think) and update the branch here in the meantime.
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.
Thanks @federicocappelli! It's great to have this change approved. Once we the JS review and the monitoring discussion are resolved, I'll do a final merge.
@federicocappelli I've added the parameter to the breakage pixel too. Could you check if it looks ok? |
<!-- Note: This checklist is a reminder of our shared engineering expectations. --> Please review the release process for BrowserServicesKit [here](https://app.asana.com/0/1200194497630846/1200837094583426). **Required**: Task/Issue URL: https://app.asana.com/0/1201621853593513/1207940672701584/f iOS PR: macOS PR: duckduckgo/macos-browser#3086 What kind of version bump will this require?: Major/Minor/Patch **Optional**: Tech Design URL: CC: **Description**: See the macos PR for details <!-- Tagging instructions If this PR isn't ready to be merged for whatever reason it should be marked with the `DO NOT MERGE` label (particularly if it's a draft) If it's pending Product Review/PFR, please add the `Pending Product Review` label. If at any point it isn't actively being worked on/ready for review/otherwise moving forward (besides the above PR/PFR exception) strongly consider closing it (or not opening it in the first place). If you decide not to close it, make sure it's labelled to make it clear the PRs state and comment with more information. --> **Steps to test this PR**: 1. 1. <!-- Before submitting a PR, please ensure you have tested the combinations you expect the reviewer to test, then delete configurations you *know* do not need explicit testing. Using a simulator where a physical device is unavailable is acceptable. --> **OS Testing**: * [ ] iOS 14 * [ ] iOS 15 * [ ] iOS 16 * [ ] macOS 10.15 * [ ] macOS 11 * [ ] macOS 12 --- ###### Internal references: [Software Engineering Expectations](https://app.asana.com/0/59792373528535/199064865822552) [Technical Design Template](https://app.asana.com/0/59792373528535/184709971311943)
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 use of singletons is not how we want to do this but it's ok for an experiment.
LGTM
Task/Issue URL: https://app.asana.com/0/1201621853593513/1207940672701584/f
Tech Design URL:
CC:
Description:
This adds a CPM filterlist experiment as described in Asana
Steps to test this PR:
Definition of Done:
Internal references:
Pull Request Review Checklist
Software Engineering Expectations
Technical Design Template
Pull Request Documentation