Add reboot notification setting and integration settings flows for OCPP - #1990
Add reboot notification setting and integration settings flows for OCPP#1990dominikandreas wants to merge 8 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe integration adds per-charger Home Assistant notification settings, dynamic configuration and options flows, migration updates, conditional notification dispatch, translations, documentation, workflow updates, and test coverage. ChangesConfigurable charger notifications
Workflow and test maintenance
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
tests/test_charge_point_core.py (1)
228-231: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueEnsure safe closure of the mock coroutine.
Calling an
AsyncMockreturns an awaitable that may not possess aclose()method, which could lead to anAttributeErrorwhencoro.close()is invoked. Consider checking for thecloseattribute to make the fake task creator robust.♻️ Proposed fix
def fake_async_create_task(coro): scheduled.append(coro) - coro.close() + if hasattr(coro, "close"): + coro.close() return None🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_charge_point_core.py` around lines 228 - 231, Update fake_async_create_task to close the captured coroutine only when it provides a callable close attribute, while preserving the existing scheduling and return behavior.custom_components/ocpp/translations/i-default.json (1)
14-14: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueKeep translation files synchronized.
The
optionsblock was added toen.jsonbut seems to be missing fromi-default.json. Consider adding theoptions.step.initsection here as well to keep the base translation file consistent.♻️ Proposed fix
"abort": { "single_instance_allowed": "Only a single instance is allowed", "reauth_successful": "New charger configured" } - } + }, + "options": { + "step": { + "init": { + "title": "OCPP Central System Settings", + "description": "If you need help with the configuration have a look [here]({docs_url})", + "data": { + "host": "Central system host address", + "port": "Central system port number", + "ssl": "Secure connection", + "ssl_certfile_path": "Path to SSL certificate or (None)", + "ssl_keyfile_path": "Path to SSL key or (None)", + "csid": "Central system identity", + "enable_reboot_notifications": "Enable charger reboot notifications", + "websocket_close_timeout": "Websocket close timeout (seconds)", + "websocket_ping_tries": "Websocket successive times to try connection before closing", + "websocket_ping_interval": "Websocket ping interval (seconds)", + "websocket_ping_timeout": "Websocket ping timeout (seconds)" + } + } + } + } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@custom_components/ocpp/translations/i-default.json` at line 14, Add the missing options.step.init translation section to i-default.json, matching the structure and keys already present in en.json while preserving the existing enable_reboot_notifications entry and translation-file formatting.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@custom_components/ocpp/config_flow.py`:
- Around line 327-329: Replace the `_async_abort_entries_match` call in the
options-flow port-change handling with an explicit iteration over existing
config entries, aborting when another entry in the domain already uses the
requested port. Preserve the current behavior of allowing the unchanged port and
avoid relying on the unavailable OptionsFlow method.
---
Nitpick comments:
In `@custom_components/ocpp/translations/i-default.json`:
- Line 14: Add the missing options.step.init translation section to
i-default.json, matching the structure and keys already present in en.json while
preserving the existing enable_reboot_notifications entry and translation-file
formatting.
In `@tests/test_charge_point_core.py`:
- Around line 228-231: Update fake_async_create_task to close the captured
coroutine only when it provides a callable close attribute, while preserving the
existing scheduling and return behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: cb6ec4ba-0e56-41d0-874f-141bbc23c121
📒 Files selected for processing (9)
custom_components/ocpp/chargepoint.pycustom_components/ocpp/config_flow.pycustom_components/ocpp/const.pycustom_components/ocpp/translations/en.jsoncustom_components/ocpp/translations/i-default.jsondocs/support.mdtests/const.pytests/test_charge_point_core.pytests/test_config_flow.py
GitHub Actions
Failure
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/sphinx-build.yml:
- Line 10: Update the Sphinx workflow’s ammaraskar/sphinx-action reference to a
full immutable commit SHA or an approved immutable release reference, replacing
the mutable master tag while preserving the action configuration.
- Line 9: Disable persisted checkout credentials on the checkout steps at
.github/workflows/sphinx-build.yml:9,
.github/workflows/publish_docs_to_wiki.yml:21, .github/workflows/tests.yaml:20,
.github/workflows/tests.yaml:49, .github/workflows/tests.yaml:62,
.github/workflows/tests.yaml:72, .github/workflows/labeler.yml:15, and
.github/workflows/publish.yml:19 by setting persist-credentials to false; leave
the checkout used by the version-update workflow unchanged because its later git
commit and push require credentials.
In `@custom_components/ocpp/__init__.py`:
- Around line 173-174: Update async_migrate_entry() so the version-2 migration
backfills CONF_ENABLE_REBOOT_NOTIFICATIONS and CONF_OCPP_VERSION for existing
version-2, minor_version=0 entries, alongside the existing CONF_CPIDS update.
Advance the migration minor version accordingly and add regression coverage for
a pre-existing version-2 entry.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: f4eb06ac-1f7e-4749-8a07-c3e805419c51
📒 Files selected for processing (16)
.github/workflows/labeler.yml.github/workflows/publish.yml.github/workflows/publish_docs_to_wiki.yml.github/workflows/release-drafter.yml.github/workflows/sphinx-build.yml.github/workflows/stale.yml.github/workflows/tests.yamlcustom_components/ocpp/__init__.pycustom_components/ocpp/chargepoint.pycustom_components/ocpp/config_flow.pycustom_components/ocpp/const.pycustom_components/ocpp/translations/en.jsoncustom_components/ocpp/translations/i-default.jsontests/const.pytests/test_charge_point_core.pytests/test_config_flow.py
🚧 Files skipped from review as they are similar to previous changes (6)
- custom_components/ocpp/chargepoint.py
- custom_components/ocpp/const.py
- custom_components/ocpp/translations/en.json
- tests/test_charge_point_core.py
- tests/const.py
- custom_components/ocpp/config_flow.py
| runs-on: ubuntu-latest | ||
| steps: | ||
| - uses: actions/checkout@v7 | ||
| - uses: actions/checkout@v7 |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== workflow files =="
git ls-files '.github/workflows/*'
echo
echo "== checkout occurrences =="
rg -n -C 2 'actions/checkout' .github/workflows || true
echo
echo "== token mentions =="
rg -n -C 2 'GITHUB_TOKEN|USER_TOKEN|persist-credentials' .github/workflows || true
echo
echo "== inspect affected files sizes and relevant lines =="
for f in .github/workflows/sphinx-build.yml .github/workflows/publish_docs_to_wiki.yml .github/workflows/tests.yaml .github/workflows/labeler.yml .github/workflows/publish.yml; do
echo "--- $f ($(wc -l < "$f") lines) ---"
nl -ba "$f" | sed -n '1,100p'
done
echo
echo "== local action references =="
rg -n 'uses:\s*\.|permissions:|fetch-depth|persist-credentials|GITHUB_TOKEN|USER_TOKEN|secrets\.' .github/workflows || trueRepository: lbbrhzn/ocpp
Length of output: 5524
🌐 Web query:
actions/checkout persist-credentials default true GITHUB_TOKEN local git configuration
💡 Result:
In actions/checkout, the persist-credentials input determines whether the authentication token (such as GITHUB_TOKEN) or SSH key used to fetch the repository is configured in the local git environment [1][2]. Key details regarding this setting include: Default Behavior: The default value for persist-credentials is true [1][3]. When enabled, this allows subsequent steps in your job to run authenticated git commands (e.g., git push or git fetch) without needing to manually re-authenticate [1][2]. Security Architecture: 1. Historical Implementation: In earlier versions, credentials were persisted directly in the local.git/config file [4][5]. 2. Updated Implementation: More recent versions (e.g., v6.0.0 and newer) have updated the mechanism to store credentials in a separate file under $RUNNER_TEMP rather than directly in the git configuration file [3][4]. This change reduces the risk of accidental exposure while maintaining functionality [3][4]. 3. Cleanup: The action includes a post-job step designed to remove these persisted credentials, ensuring they do not persist beyond the lifecycle of the job [1][2]. Security Considerations: While the action attempts to manage credentials securely, setting persist-credentials to false is a commonly recommended hardening practice for workflows that do not require authenticated git operations in subsequent steps [6][7]. This opt-out approach minimizes the exposure of credentials within the job environment [2]. Note that the GITHUB_TOKEN is available to all steps in a job regardless of this setting; persist-credentials specifically controls whether that token is implicitly configured for git commands [7].
Citations:
- 1: https://github.com/actions/checkout/blob/cbb722410c2e876e24abbe8de2cc27693e501dcb/action.yml
- 2: https://github.com/marketplace/actions/checkout?version=v3.6.0
- 3: https://github.com/actions/checkout/tree/v6.0.0
- 4: https://github.com/actions/checkout?v=1.1.1
- 5: https://github.com/actions/checkout/blob/v2.4.0/README.md
- 6: Change the default value of persist-credentials to false actions/checkout#1687
- 7: Remove
persist-credentialsor change the default tofalseactions/checkout#485
🌐 Web query:
actions/checkout v7 persist-credentials default true README
💡 Result:
In the actions/checkout action, the persist-credentials input has a default value of true [1][2][3]. When set to true, the action configures the Git credential helper or local Git configuration with the token or SSH key used for the checkout [1][4]. This persistence allows subsequent steps in your workflow to execute authenticated Git commands (such as git fetch or git push) without needing to manually re-authenticate [1][5]. The credentials are removed during the post-job cleanup [1][5]. Starting with v6, the implementation of this feature was updated for improved security: rather than being stored directly in the local.git/config file, credentials are now stored in a separate file located under $RUNNER_TEMP [5][6][7]. To opt out of this behavior, you can explicitly set the input to false in your workflow file [1][8]: - uses: actions/checkout@v7 with: persist-credentials: false
Citations:
- 1: https://github.com/actions/checkout?tab=readme-ov-file
- 2: https://github.com/actions/checkout/blob/34e114876b0b11c390a56381ad16ebd13914f8d5/action.yml
- 3: https://github.com/actions/checkout/blob/cbb722410c2e876e24abbe8de2cc27693e501dcb/action.yml
- 4: https://github.com/marketplace/actions/checkout?version=v3.6.0
- 5: https://github.com/actions/checkout/tree/v6.0.2?tab=readme-ov-file
- 6: https://github.com/actions/checkout/tree/v6.0.0
- 7: actions/checkout@v5.0.1...v6.0.0
- 8: https://github.com/actions/checkout/?tab=readme-ov-file
Disable persisted checkout credentials where subsequent steps use explicit tokens.
actions/checkout persists the GitHub credential by default. Disable it in workflows where later steps authenticate with another token or do not need authenticated Git commands, and keep it only in workflows that require authenticated git operations after checkout.
- Keep checkout credentials in
.github/workflows/update-version.yml; the latergit commit/git pushneeds an authenticated token there. - Set
persist-credentials: falseon the checkout steps in.github/workflows/publish_docs_to_wiki.yml, because the wiki Git commands useUSER_TOKEN. - For the remaining checkout steps, disable persistence unless an authenticated Git command runs in the same job after checkout.
🧰 Tools
🪛 zizmor (1.28.0)
[warning] 9-9: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
📍 Affects 5 files
.github/workflows/sphinx-build.yml#L9-L9(this comment).github/workflows/publish_docs_to_wiki.yml#L21-L21.github/workflows/tests.yaml#L20-L20.github/workflows/tests.yaml#L49-L49.github/workflows/tests.yaml#L62-L62.github/workflows/tests.yaml#L72-L72.github/workflows/labeler.yml#L15-L15.github/workflows/publish.yml#L19-L19
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/sphinx-build.yml at line 9, Disable persisted checkout
credentials on the checkout steps at .github/workflows/sphinx-build.yml:9,
.github/workflows/publish_docs_to_wiki.yml:21, .github/workflows/tests.yaml:20,
.github/workflows/tests.yaml:49, .github/workflows/tests.yaml:62,
.github/workflows/tests.yaml:72, .github/workflows/labeler.yml:15, and
.github/workflows/publish.yml:19 by setting persist-credentials to false; leave
the checkout used by the version-update workflow unchanged because its later git
commit and push require credentials.
Source: Linters/SAST tools
|
Options and reconfigure flows are now included, for notifications the functionality should be generalised to all HA notifications and set for each charger. |
# Conflicts: # .github/workflows/release-drafter.yml # custom_components/ocpp/__init__.py # custom_components/ocpp/config_flow.py # tests/test_init.py
|
Please start with a clean PR as it is difficult to separate your changes from those already in main |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
custom_components/ocpp/chargepoint.py (1)
492-494: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftMake the post-connect backstop session-scoped and single-flight.
post_connect_successis set after the first successful session and is not reset inreconnectat Lines 594-603. A later connection without a boot notification therefore skips this backstop.During the initial connection,
_register_boot_notificationat Lines 626-630 can already have apost_connect()task running when this timer expires. Both paths can then start duplicate initialization.Track one post-connect task per connection, cancel it during stop/reconnect, and reset the session state before starting the new monitor.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@custom_components/ocpp/chargepoint.py` around lines 492 - 494, Make the post-connect backstop session-scoped and single-flight: reset post_connect_success before each new monitor/session, cancel and clear any existing post_connect task during stop/reconnect, and ensure both the monitor backstop and _register_boot_notification reuse or guard the same per-connection task instead of starting duplicates. Update the relevant reconnect/stop and post-connect scheduling logic while preserving successful initialization behavior.
🧹 Nitpick comments (2)
tests/test_options_flow.py (1)
387-416: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the stored default for
CONF_ENABLE_HA_NOTIFICATIONS.This test checks defaults for existing fields but not the new notification field. If the default regresses to
DEFAULT_ENABLE_HA_NOTIFICATIONS, saving a charger whose stored value isFalsecan re-enable notifications.Add a stored
Falsevalue and assert that the form default isFalse. This follows the default lookup incustom_components/ocpp/config_flow.py.Suggested test extension
CONF_MAX_CURRENT: 63, CONF_SKIP_SCHEMA_VALIDATION: True, CONF_FORCE_SMART_CHARGING: False, + CONF_ENABLE_HA_NOTIFICATIONS: False, } ) } ... assert defaults[CONF_FORCE_SMART_CHARGING] is False + assert defaults[CONF_ENABLE_HA_NOTIFICATIONS] is False🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_options_flow.py` around lines 387 - 416, Extend test_the_form_defaults_are_the_stored_values by storing CONF_ENABLE_HA_NOTIFICATIONS as False in _cp_settings and asserting the corresponding data_schema default is False, verifying the options form uses the persisted value rather than DEFAULT_ENABLE_HA_NOTIFICATIONS.custom_components/ocpp/chargepoint.py (1)
670-675: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winTrack targeted-refresh readiness per metric set.
The latency caller resolves
[cstat.latency_ping.value, cstat.latency_pong.value], while heartbeat callers resolve[cstat.heartbeat.value]. A successful latency refresh can set_targeted_refresh_ready; a later missing heartbeat entity then skips the full-update fallback.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@custom_components/ocpp/chargepoint.py` around lines 670 - 675, Update the targeted-refresh readiness logic around _targeted_refresh_ready so readiness is tracked separately for each requested metric set rather than as one global flag. Ensure the latency and heartbeat refresh paths only reuse readiness for their own entity set, while a missing metric set still triggers update(self.settings.cpid) when fallback_to_full_update is enabled.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@custom_components/ocpp/chargepoint.py`:
- Around line 492-494: Make the post-connect backstop session-scoped and
single-flight: reset post_connect_success before each new monitor/session,
cancel and clear any existing post_connect task during stop/reconnect, and
ensure both the monitor backstop and _register_boot_notification reuse or guard
the same per-connection task instead of starting duplicates. Update the relevant
reconnect/stop and post-connect scheduling logic while preserving successful
initialization behavior.
---
Nitpick comments:
In `@custom_components/ocpp/chargepoint.py`:
- Around line 670-675: Update the targeted-refresh readiness logic around
_targeted_refresh_ready so readiness is tracked separately for each requested
metric set rather than as one global flag. Ensure the latency and heartbeat
refresh paths only reuse readiness for their own entity set, while a missing
metric set still triggers update(self.settings.cpid) when
fallback_to_full_update is enabled.
In `@tests/test_options_flow.py`:
- Around line 387-416: Extend test_the_form_defaults_are_the_stored_values by
storing CONF_ENABLE_HA_NOTIFICATIONS as False in _cp_settings and asserting the
corresponding data_schema default is False, verifying the options form uses the
persisted value rather than DEFAULT_ENABLE_HA_NOTIFICATIONS.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 37839fe5-0592-43e6-b884-d15292f38466
📒 Files selected for processing (10)
.github/workflows/release-drafter.ymlcustom_components/ocpp/__init__.pycustom_components/ocpp/chargepoint.pycustom_components/ocpp/config_flow.pycustom_components/ocpp/const.pycustom_components/ocpp/translations/en.jsoncustom_components/ocpp/translations/i-default.jsontests/test_config_flow.pytests/test_init.pytests/test_options_flow.py
🚧 Files skipped from review as they are similar to previous changes (5)
- .github/workflows/release-drafter.yml
- custom_components/ocpp/const.py
- custom_components/ocpp/init.py
- tests/test_init.py
- custom_components/ocpp/config_flow.py
|
Superseded by #2068, rebuilt directly from the current upstream main branch as a single feature-only commit, as requested. Closing this PR in favor of the clean replacement. |
2 participants
This PR adds two new features:
OptionsFlowhandler to configure settings from Home Assistant’s Integrations page after initial setupThe new reboot notification setting is now included in the create flow, reconfigure flow, and integration options flow. Alongside that, this PR introduces support for reconfiguring an existing OCPP entry and adds an options flow so users can update settings such as host, port, SSL, central system ID, websocket timing values, and the new reboot notification behavior without recreating the integration.
Screenshot of the new feature for changing the integration settings:
The changes were implemented with copilot and GPT 5.4. I've only reviewed and tested them in my own installation.
Related issue: #1425
Summary by CodeRabbit
New Features
Bug Fixes
Documentation