-
-
Notifications
You must be signed in to change notification settings - Fork 39
Add disable protection for a specific duration #1573
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
base: main
Are you sure you want to change the base?
Conversation
|
@rbeuque74 Thank you for submitting this! Suggest adding a label so the PR passes the first check. According to the workflow output, valid labels are: ['breaking-change', 'bugfix', 'documentation', 'enhancement', 'refactor', 'performance', 'new-feature', 'maintenance', 'ci', 'dependencies'] Maybe enhancement or new-feature? |
|
@dbensmith in my understanding, @frenck configured the repository as only the maintainers can change the labels on pull requests. Or there is some automations based on commit message/pull request description that auto set the labels ? 🤷 |
This comment was marked as resolved.
This comment was marked as resolved.
|
Hello |
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1573 +/- ##
===========================================
- Coverage 100.00% 99.41% -0.59%
===========================================
Files 9 10 +1
Lines 316 339 +23
Branches 26 17 -9
===========================================
+ Hits 316 337 +21
- Partials 0 2 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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.
Pull Request Overview
This PR adds support for temporarily disabling AdGuard Home protection for a specific duration, aligning with functionality introduced in AdGuard Home v0.107.28. The implementation updates both the API endpoint and payload format to use the newer /control/protection endpoint.
Key Changes:
- Added optional
pause_duration_milisecondsparameter todisable_protection()method - Updated API endpoints from
/control/dns_configto/control/protectionfor both enable and disable operations - Updated JSON payload format from
protection_enabledtoenabledkey - Fixed typo in test function name (
test_verion→test_version)
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| src/adguardhome/adguardhome.py | Adds optional duration parameter to disable_protection(), updates API endpoint and payload format for protection methods |
| tests/test_adguardhome.py | Updates test mocks to use new /control/protection endpoint and fixes test function name typo |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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.
Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.
frenck
left a comment
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.
Oh hi there @rbeuque74 👋
Thanks for the pull request. I think Copilot has raised a few valid concerns in the above review. Could you take a look?
Thanks! 👍
../Frenck
Blogging my personal ramblings at frenck.dev
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.
Pull Request Overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
9517fd8 to
842848b
Compare
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.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Fixes frenck#1030 Signed-off-by: Romain Beuque <[email protected]>
842848b to
e7bc299
Compare
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.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Hey @frenck, Romain |
Proposed Changes
Since AGH v0.107.28, AGH allow to disable protection for a specific duration (see AdguardTeam/AdGuardHome#1333).
This option is not present in
disable_protection, hence, cannot be used in Home Assistant, using aservice.This PR introduces having an optional
pause_duration_milisecondsparameter.Related Issues
#1030