Follow Core PR 135653 for 2025.2.0 (moving ZeroconfServiceInfo) #822
Conversation
CoMPaTech
added
bug
WalkthroughThis pull request introduces several updates across multiple configuration and metadata files for the Plugwise beta integration. The changes include version updates in Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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 (1)
custom_components/plugwise/config_flow.py (1)
Line range hint
149-153: Consider replacing broad exception handling with specific exceptions.The broad
Exceptioncatch block could mask unexpected errors. Consider:
- Listing all possible exceptions that could be raised by
validate_input- Handling them explicitly to provide more accurate error messages
Example refactor:
- except Exception: - _LOGGER.exception( - "Unknown exception while verifying connection with your Plugwise Smile" - ) - errors[CONF_BASE] = "unknown" + except OSError as err: + _LOGGER.exception("Network error while connecting to Plugwise Smile: %s", err) + errors[CONF_BASE] = "cannot_connect" + except TimeoutError as err: + _LOGGER.exception("Timeout while connecting to Plugwise Smile: %s", err) + errors[CONF_BASE] = "cannot_connect"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
.gitignore(1 hunks)CHANGELOG.md(1 hunks)custom_components/plugwise/config_flow.py(2 hunks)custom_components/plugwise/coordinator.py(1 hunks)custom_components/plugwise/manifest.json(1 hunks)hacs.json(1 hunks)tests/components/plugwise/conftest.py(1 hunks)tests/components/plugwise/test_binary_sensor.py(1 hunks)tests/components/plugwise/test_climate.py(1 hunks)tests/components/plugwise/test_config_flow.py(4 hunks)tests/components/plugwise/test_diagnostics.py(1 hunks)tests/components/plugwise/test_init.py(1 hunks)
✅ Files skipped from review due to trivial changes (9)
- .gitignore
- custom_components/plugwise/manifest.json
- custom_components/plugwise/coordinator.py
- tests/components/plugwise/test_binary_sensor.py
- tests/components/plugwise/conftest.py
- tests/components/plugwise/test_init.py
- tests/components/plugwise/test_diagnostics.py
- tests/components/plugwise/test_climate.py
- tests/components/plugwise/test_config_flow.py
🔇 Additional comments (3)
hacs.json (1)
12-12: Verify Home Assistant version compatibility.The specified version "2025.2.0" is a future version. Please ensure this version requirement aligns with the actual Home Assistant release schedule and compatibility needs.
Run the following script to check the latest available Home Assistant version:
✅ Verification successful
Home Assistant version requirement is valid
The specified version "2025.2.0" correctly targets the upcoming February 2025 release, following Home Assistant's monthly release cycle. The current latest version is 2025.1.4 (January 2025).
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check latest Home Assistant version from PyPI. curl -s https://pypi.org/pypi/homeassistant/json | jq '.info.version'Length of output: 80
CHANGELOG.md (1)
5-10: LGTM! Well-documented changelog entry.The changelog entry follows the established format and provides clear descriptions of the changes, including:
- Internal CI process fixes
- Test coverage improvements for binary_sensor
- Config flow enhancements
- Reference to Core PR for ZeroconfServiceInfo changes
custom_components/plugwise/config_flow.py (1)
Line range hint
83-83: LGTM! Type hints correctly updated.The type hints for
ZeroconfServiceInfoare properly updated in both thesmile_user_schemafunction andasync_step_zeroconfmethod.Also applies to: 165-165
CoMPaTech
changed the title
bouwew
left a comment
bouwew
left a comment
There was a problem hiding this comment.
LGTM!
Is there a way to force the Action to test vs dev-branch, to make the testing all pass?
|
I was thinking the same last night, we already disregard failing 'dev' in normal builds (but we still evaluate) - so we should invert it, labels could/would do this but requires setting them correctly (but than the action on last commit would already run). |
CoMPaTech
added
the
require-dev-pass
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
.github/workflows/test.yml (4)
69-76: Document the label functionality in the workflow.The new label-based control for test strictness is a good addition. Consider adding a comment block explaining:
- The purpose of the
require-dev-passlabel- When to use it
- Its impact on the workflow
Add this documentation above the step:
+ # Controls test strictness based on PR labels: + # - 'require-dev-pass': When present, dev tests must pass + # - Otherwise: Dev test failures are allowed - name: Determine Branch Test Mode
86-86: Consider adding environment type to cache version key.While separating caches for release and dev is good, consider including the environment type in the cache version key for better isolation:
env: - CACHE_VERSION: 29 + CACHE_VERSION_RELEASE: 29 + CACHE_VERSION_DEV: 29Then update the cache keys:
- ${{ env.CACHE_VERSION}}-${{ runner.os }}-base-hacore- + ${{ env.CACHE_VERSION_RELEASE}}-${{ runner.os }}-base-hacore- # for release job + ${{ env.CACHE_VERSION_DEV}}-${{ runner.os }}-base-hacore- # for dev jobAlso applies to: 129-129
100-102: Enhance error messaging for test failures.The conditional test failure handling is good, but consider making the failure messages more informative:
- name: Test through HA-core (master/release) continue-on-error: ${{ env.STRICT_DEV == 'true' }} run: | + echo "Note: Test failures will ${{ env.STRICT_DEV == 'true' && 'be allowed' || 'cause workflow failure' }}" GITHUB_ACTIONS="" scripts/core-testing.sh - name: Test through HA-core (dev) continue-on-error: ${{ env.STRICT_DEV == 'false' }} run: | + echo "Note: Test failures will ${{ env.STRICT_DEV == 'false' && 'be allowed' || 'cause workflow failure' }}" GITHUB_ACTIONS="" BRANCH="dev" scripts/core-testing.shAlso applies to: 143-145
104-145: Consider reducing job duplication using a reusable workflow.The
ha-core-devjob largely duplicates theha-core-releasejob structure. Consider extracting the common logic into a reusable workflow:
- Create
.github/workflows/ha-core-test.yml:name: Reusable HA Core Test on: workflow_call: inputs: branch: required: true type: string cache-suffix: required: true type: string jobs: test: runs-on: ubuntu-latest steps: - uses: actions/checkout@v4.2.2 # ... common steps ... - name: Test through HA-core continue-on-error: ${{ env.STRICT_DEV == (inputs.branch == 'dev' ? 'false' : 'true') }} run: | GITHUB_ACTIONS="" BRANCH="${{ inputs.branch }}" scripts/core-testing.sh
- Update the main workflow to use it:
jobs: ha-core-release: uses: ./.github/workflows/ha-core-test.yml with: branch: master cache-suffix: release ha-core-dev: uses: ./.github/workflows/ha-core-test.yml with: branch: dev cache-suffix: dev
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/test.yml(3 hunks).pre-commit-config.yaml(1 hunks)
🔇 Additional comments (3)
.pre-commit-config.yaml (2)
Line range hint
2-4: Verify Python version compatibility.The specified Python version 3.13 is not yet released. This might cause issues as it's not available in most environments.
Consider using Python 3.11 or 3.12 instead, which are stable releases.
80-80: LGTM! Consistent environment variable handling.The addition of
BRANCH="${BRANCH:-}"to the entry commands provides a clean way to optionally specify a branch while defaulting to an empty value when not set. This change aligns well with the PR objectives regarding build process improvements.Let's verify the script's handling of the BRANCH variable:
Also applies to: 87-87, 94-94, 101-101
✅ Verification successful
✅ BRANCH variable handling verification successful
The core-testing.sh script correctly handles both set and unset BRANCH variables, working seamlessly with the pre-commit hook's
${BRANCH:-}pattern. The implementation allows for optional branch specification while maintaining a clean default behavior.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the core-testing.sh script's handling of the BRANCH variable # Check if the script exists and how it uses the BRANCH variable if [ -f "scripts/core-testing.sh" ]; then echo "Checking BRANCH variable usage in core-testing.sh:" rg -A 3 'BRANCH' scripts/core-testing.sh fiLength of output: 473
.github/workflows/test.yml (1)
61-61: LGTM! Clear job naming and description.The job rename from
ha-core-preparetoha-core-releasebetter reflects its purpose, and the description clearly indicates its scope for release/master branches.Also applies to: 63-63
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
.github/workflows/test.yml (3)
69-76: Consider making the label check more robust.While the label-based control is a good approach, consider these improvements:
- Add a comment explaining the purpose of the 'require-dev-pass' label
- Consider handling the case when github.event.pull_request is undefined (e.g., for push events)
- name: Determine Branch Test Mode id: determine_mode run: | - if [[ "${{ contains(github.event.pull_request.labels.*.name, 'require-dev-pass') }}" == "true" ]]; then + # Set STRICT_DEV=true when 'require-dev-pass' label is present, enforcing dev branch tests to pass + if [[ "${{ github.event_name }}" == "pull_request" && \ + "${{ contains(github.event.pull_request.labels.*.name, 'require-dev-pass') }}" == "true" ]]; then echo "STRICT_DEV=true" >> $GITHUB_ENV else echo "STRICT_DEV=false" >> $GITHUB_ENV fi
100-102: Document the test failure behavior configuration.While the implementation is correct, consider adding workflow-level documentation explaining:
- When dev/release tests are allowed to fail
- The impact of the 'require-dev-pass' label
- The relationship between STRICT_DEV and continue-on-error
Add this documentation at the top of the workflow:
# Test failure behavior: # - Release tests: Allowed to fail only when require-dev-pass label is present # - Dev tests: Allowed to fail by default, must pass when require-dev-pass label is presentAlso applies to: 143-145
104-145: Consider using reusable workflows to reduce duplication.The ha-core-dev job largely duplicates the ha-core-release job structure. Consider extracting common steps into a reusable workflow to improve maintainability.
Example approach:
- Create a new file
.github/workflows/ha-core-test.yml:name: Reusable HA Core Test on: workflow_call: inputs: environment: required: true type: string branch: required: true type: string jobs: test: runs-on: ubuntu-latest steps: - uses: actions/checkout@v4.2.2 # ... common steps ... - name: Test through HA-core continue-on-error: ${{ inputs.environment == 'release' && env.STRICT_DEV == 'true' || inputs.environment == 'dev' && env.STRICT_DEV == 'false' }} run: | GITHUB_ACTIONS="" BRANCH="${{ inputs.branch }}" scripts/core-testing.sh
- Update the main workflow to use the reusable workflow:
jobs: ha-core-release: uses: ./.github/workflows/ha-core-test.yml with: environment: release branch: master ha-core-dev: uses: ./.github/workflows/ha-core-test.yml with: environment: dev branch: dev
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
.github/workflows/test.yml(3 hunks).gitignore(1 hunks).pre-commit-config.yaml(1 hunks)CHANGELOG.md(1 hunks)custom_components/plugwise/config_flow.py(2 hunks)custom_components/plugwise/coordinator.py(0 hunks)hacs.json(1 hunks)pyproject.toml(1 hunks)tests/components/plugwise/conftest.py(0 hunks)tests/components/plugwise/test_binary_sensor.py(1 hunks)tests/components/plugwise/test_climate.py(1 hunks)tests/components/plugwise/test_config_flow.py(4 hunks)tests/components/plugwise/test_diagnostics.py(1 hunks)tests/components/plugwise/test_init.py(1 hunks)
💤 Files with no reviewable changes (2)
- custom_components/plugwise/coordinator.py
- tests/components/plugwise/conftest.py
🚧 Files skipped from review as they are similar to previous changes (11)
- .gitignore
- hacs.json
- tests/components/plugwise/test_binary_sensor.py
- tests/components/plugwise/test_diagnostics.py
- pyproject.toml
- custom_components/plugwise/config_flow.py
- tests/components/plugwise/test_climate.py
- tests/components/plugwise/test_init.py
- tests/components/plugwise/test_config_flow.py
- .pre-commit-config.yaml
- CHANGELOG.md
🔇 Additional comments (2)
.github/workflows/test.yml (2)
61-61: LGTM! Clear and descriptive job naming.The rename from 'ha-core-prepare' to 'ha-core-release' better reflects the job's purpose, improving workflow clarity.
Also applies to: 63-63
86-86: LGTM! Good cache separation practice.Separating caches for release and dev environments prevents cache pollution and improves build reliability.
Also applies to: 129-129
…ilding for HA dev branch
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
.github/workflows/test.yml (1)
Line range hint
104-145: Consider adding job dependency on ha-core-release.The new
ha-core-devjob is well structured and mirrors the release job configuration. However, consider whether it should run afterha-core-releasecompletes to optimize workflow execution time and resource usage.Apply this diff to add the job dependency:
needs: - pre-commit + - ha-core-release
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
.github/workflows/test.yml(3 hunks).gitignore(1 hunks).pre-commit-config.yaml(1 hunks)CHANGELOG.md(1 hunks)custom_components/plugwise/config_flow.py(1 hunks)custom_components/plugwise/manifest.json(1 hunks)hacs.json(1 hunks)pyproject.toml(1 hunks)tests/components/plugwise/test_config_flow.py(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- custom_components/plugwise/manifest.json
🚧 Files skipped from review as they are similar to previous changes (7)
- .gitignore
- hacs.json
- CHANGELOG.md
- pyproject.toml
- tests/components/plugwise/test_config_flow.py
- custom_components/plugwise/config_flow.py
- .pre-commit-config.yaml
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Setup for HA-core (release/master)
- GitHub Check: Setup for HA-core (dev)
🔇 Additional comments (5)
.github/workflows/test.yml (5)
61-63: LGTM! Clear job renaming and description.The job renaming from
ha-core-preparetoha-core-releasewith an updated description better reflects its purpose of handling both release and master branches.
86-98: LGTM! Improved cache isolation.The cache naming update (
cache-hacore-release) provides better isolation between environments and prevents cache conflicts.
99-102: Verify the environment variable handling in the script.The test execution configuration looks good, but ensure that the
core-testing.shscript properly handles the emptyGITHUB_ACTIONSenvironment variable.✅ Verification successful
The empty GITHUB_ACTIONS variable is properly handled
The script demonstrates robust handling of empty GITHUB_ACTIONS variable through proper shell parameter expansion and consistent conditional checks across all usage points.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if core-testing.sh handles empty GITHUB_ACTIONS variable # Look for GITHUB_ACTIONS variable usage rg -A 3 'GITHUB_ACTIONS' scripts/core-testing.shLength of output: 1224
69-76: Verify the label configuration in repository settings.The new label-based control for test failure handling is a good addition. However, ensure that the
require-dev-passlabel is properly configured in the repository settings.✅ Verification successful
Label
require-dev-passis properly configured ✓The label exists in the repository with the correct name and has an appropriate description that matches its intended functionality in the workflow.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the label configuration in repository settings # Check if the label exists and its configuration gh api repos/plugwise/plugwise-beta/labels | jq '.[] | select(.name=="require-dev-pass")'Length of output: 421
142-145: Verify the BRANCH environment variable handling.The
BRANCH="dev"environment variable is correctly set, but ensure that thecore-testing.shscript properly handles this variable.✅ Verification successful
BRANCH environment variable is properly handled
The
core-testing.shscript correctly handles the BRANCH variable through proper validation and usage:
- Sets default to "master" if BRANCH is not set
- Properly validates and uses the provided BRANCH value
- Consistently applies it for all git operations
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if core-testing.sh handles BRANCH variable # Look for BRANCH variable usage and validation rg -A 3 'BRANCH=' scripts/core-testing.shLength of output: 41
Script:
#!/bin/bash # First verify the script exists ls -l scripts/core-testing.sh # View the content of the script cat scripts/core-testing.sh # Search for BRANCH with broader patterns rg -i "branch" scripts/core-testing.shLength of output: 11395
|
Ok, so pushing no longer allowed, PRs allowed - although not shown actually 'master' fails while 'dev' works. Now removing label to see if it breaks. |
CoMPaTech
removed
the
require-dev-pass
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
.github/workflows/test.yml (1)
10-17: Consider implementing the commented workaround for push events.The commented code shows a potential solution for handling labels in push events. If you plan to implement this, ensure proper error handling for API calls and consider rate limiting.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/hassfest.yaml(1 hunks).github/workflows/test.yml(4 hunks)
🔇 Additional comments (5)
.github/workflows/hassfest.yaml (1)
3-8: LGTM! Workflow trigger configuration is appropriate.The removal of the
pushtrigger with the added comment helps prevent duplicate workflow runs while maintaining necessary triggers for pull requests, manual dispatches, and scheduled validations..github/workflows/test.yml (4)
19-24: LGTM! Event triggers are well-defined.The pull request triggers are appropriately configured to handle label-related events, which is crucial for the new label-based control mechanism.
80-87: LGTM! Label-based control mechanism is well implemented.The implementation using the
require-dev-passlabel andSTRICT_DEVenvironment variable provides a clean way to control build failure handling. This aligns well with the PR objectives regarding the inversion of dev build handling.Also applies to: 123-130
110-114: Verify the inverse relationship between dev and release build failures.The configuration creates an interesting dynamic:
- Release (master) failures are allowed only when dev is strict (
STRICT_DEV == 'true')- Dev failures are allowed unless dev is strict (
STRICT_DEV == 'false')This implements the intended inversion mentioned in the PR objectives, but please verify this is the desired behavior.
Also applies to: 153-156
97-109: LGTM! Cache separation is well implemented.Good practice to separate caches for release and dev builds using distinct cache names (
cache-hacore-releaseandcache-hacore-dev). This prevents potential issues from cross-contamination between environments.Also applies to: 136-152
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
.github/workflows/test.yml (1)
10-17: Remove commented-out code.The commented-out code snippet for label access is not being used and should be removed to maintain cleaner workflow files. If this is intended for future reference, consider documenting it in a separate development notes file.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/test.yml(4 hunks)
🧰 Additional context used
🪛 actionlint (1.7.4)
.github/workflows/test.yml
85-85: property "ha_core_tests" is not defined in object type {cache-hacore: {conclusion: string; outcome: string; outputs: {string => string}}; determine_mode: {conclusion: string; outcome: string; outputs: {string => string}}; ha-core-tests: {conclusion: string; outcome: string; outputs: {string => string}}; python: {conclusion: string; outcome: string; outputs: {string => string}}}
(expression)
138-138: property "ha_core_tests" is not defined in object type {cache-hacore: {conclusion: string; outcome: string; outputs: {string => string}}; determine_mode: {conclusion: string; outcome: string; outputs: {string => string}}; ha-core-tests: {conclusion: string; outcome: string; outputs: {string => string}}; python: {conclusion: string; outcome: string; outputs: {string => string}}}
(expression)
🔇 Additional comments (1)
.github/workflows/test.yml (1)
19-24: LGTM! Well-defined PR trigger events.The trigger events are properly configured to handle PR-related events including label changes, which is essential for the new label-based control flow.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
.github/workflows/test.yml (3)
85-85:⚠️ Potential issueFix the step ID reference in job outputs.
The job output references
steps.ha_core_tests.outputs.failed, but the step ID in the workflow isha_core_tests. This mismatch will cause the output to be undefined.
138-138:⚠️ Potential issueFix the step ID reference and improve error handling.
The test step has the same issues as the release job.
Also applies to: 172-183
234-249:⚠️ Potential issueFix shell script comparison and API request syntax.
There are several issues in the final comment logic:
- Shell script comparison should use
$FAIL_COUNT- The event type for requesting changes should be
REQUEST_CHANGES
🧹 Nitpick comments (3)
.github/workflows/test.yml (3)
11-16: Remove or implement the commented workaround code.The commented code block contains a potential workaround for accessing PR labels but is not implemented. Either implement the workaround or remove the commented code to maintain cleaner workflow files.
-# Workaround could be something like -# - name: Get PR labels -# run: | -# PR_LABELS=$(curl -s -H "Authorization: token ${{ secrets.GITHUB_TOKEN }}" \ -# "https://api.github.com/repos/${{ github.repository }}/issues/${{ github.event.pull_request.number }}/labels") -# echo "PR Labels: $PR_LABELS"
119-130: Improve error handling in the test step.The test step should explicitly set the output variable to ensure proper job output handling.
- name: Test through HA-core (master/release) - continue-on-error = ${{ env.STRICT_DEV == 'true' }} - id: ha_core_tests + id: ha-core-tests continue-on-error: ${{ env.STRICT_DEV == 'true' }} # Allow master failures only if dev is strict run: | GITHUB_ACTIONS="" scripts/core-testing.sh if [ $? -ne 0 ]; then echo "::warning::Release HA core incompatibility" echo "RELEASE_TESTS_FAILED=true" >> $GITHUB_ENV + echo "failed=true" >> $GITHUB_OUTPUT else echo "Successfully tested against released HA-core" echo "RELEASE_TESTS_FAILED=false" >> $GITHUB_ENV + echo "failed=false" >> $GITHUB_OUTPUT fi
205-232: Simplify the comment body construction logic.The comment body construction can be simplified by using arrays and avoiding repetitive string concatenation.
- COMMENT_BODY="**Error while testing for RELEASED HA-core:**\n" - if [[ $DEV_TESTS_FAILED == "true" ]]; then - COMMENT_BODY="**Error while testing for Development HA-core:**\n" - fi + # Define status messages + declare -A STATUS_MESSAGES=( + ["dev,true,true"]="**Error:** Incompatible while testing against dev HA-core and required to pass." + ["dev,false,true"]="**Warning:** Incompatible while testing against dev HA-core." + ["dev,any,false"]="**Success:** No problem with testing against dev HA-core." + ["release,false,true"]="**Error:** Incompatible while testing against released HA-core and required to pass." + ["release,true,true"]="**Warning:** Incompatible while testing against released HA-core." + ["release,any,false"]="**Success:** No problem with testing against released HA-core." + ) + + # Build comment body + COMMENT_BODY="### Test Results\n\n" + + # Add dev status + key="dev,$STRICT_DEV,$DEV_TESTS_FAILED" + key="${key/,any,/${DEV_TESTS_FAILED}}" + COMMENT_BODY+="${STATUS_MESSAGES[$key]}\n" + + # Add release status + key="release,$STRICT_DEV,$RELEASE_TESTS_FAILED" + key="${key/,any,/${RELEASE_TESTS_FAILED}}" + COMMENT_BODY+="${STATUS_MESSAGES[$key]}\n"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/test.yml(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Setup for HA-core (release/master)
- GitHub Check: Setup for HA-core (dev)
🔇 Additional comments (1)
.github/workflows/test.yml (1)
19-24: LGTM! Well-defined PR trigger events.The PR trigger events are well-defined and appropriate for label-based workflow control.
CoMPaTech
added
the
require-dev-pass
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
Error while testing for Development HA-core: Warning: Incompatible while testing against dev HA-core. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
.github/workflows/test.yml (3)
10-17: Consider removing commented-out code.The commented-out workaround for getting PR labels is not being used and could be removed to improve readability.
-# Do not run on 'push' (as the flow doesn't have access to the labels) - also disabled workflow_dispatch as such -# Workaround could be something like -# - name: Get PR labels -# run: | -# PR_LABELS=$(curl -s -H "Authorization: token ${{ secrets.GITHUB_TOKEN }}" \ -# "https://api.github.com/repos/${{ github.repository }}/issues/${{ github.event.pull_request.number }}/labels") -# echo "PR Labels: $PR_LABELS"
120-132: Consider improving error handling in the release tests.The result file is initialized with "true" before running tests, which could lead to false failures if the script crashes before writing the result.
- echo "true" > result.txt GITHUB_ACTIONS="" scripts/core-testing.sh if [ $? -ne 0 ]; then echo "::warning::Release HA core incompatibility" echo "true" > result.txt else echo "Successfully tested against released HA-core" echo "false" > result.txt fi
181-193: Duplicate error handling pattern in dev tests.The same error handling pattern from the release tests is duplicated here. Consider extracting this into a reusable script.
Create a new file
scripts/test-result-handler.sh:#!/bin/bash # Usage: ./test-result-handler.sh <test_command> <failure_message> <success_message> $1 if [ $? -ne 0 ]; then echo "::warning::$2" echo "true" > result.txt else echo "$3" echo "false" > result.txt fiThen update both test steps to use it:
- echo "true" > result.txt - GITHUB_ACTIONS="" BRANCH="dev" scripts/core-testing.sh - if [ $? -ne 0 ]; then - echo "::warning::Development HA core incompatibility" - echo "true" > result.txt - else - echo "Successfully tested against dev HA-core" - echo "false" > result.txt - fi + ./scripts/test-result-handler.sh \ + "GITHUB_ACTIONS="" BRANCH="dev" scripts/core-testing.sh" \ + "Development HA core incompatibility" \ + "Successfully tested against dev HA-core"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/test.yml(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Setup for HA-core (dev)
- GitHub Check: Setup for HA-core (release/master)
🔇 Additional comments (3)
.github/workflows/test.yml (3)
4-4: LGTM! Cache version increment and workflow name update.The workflow name has been updated to be more specific, and the cache version has been incremented to ensure clean caches.
Also applies to: 7-7
19-24: LGTM! Well-defined event triggers.The event triggers are properly configured to handle PR-related events, including label changes.
221-225:⚠️ Potential issueFix incorrect initial comment body logic.
The initial comment body is set based on dev test failure but uses a release test message when dev tests pass.
- if [[ $DEV_TESTS_FAILED == "true" ]]; then - COMMENT_BODY=":x: **Error while testing for Development HA-core:**\n\n" - else - COMMENT_BODY=":warning: **Warning while testing for RELEASED HA-core:**\n\n" - fi + COMMENT_BODY="# Test Results\n\n"Likely invalid or redundant comment.
CoMPaTech
added
the
require-dev-pass
|
✔️ Success: No problem with testing against dev HA-core. |
|
@bouwew took some tinkering 😊 |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
.github/workflows/test.yml (2)
11-16: Consider implementing or removing the commented workaround code.The commented code suggests a workaround for accessing PR labels, but it's left as a comment without implementation. Either implement the workaround if needed or remove the commented code to maintain cleaner workflow files.
124-132: Replace file-based result storage with step outputs.Using a file to store test results is error-prone. Consider using step outputs directly:
- echo "true" > result.txt + FAILED="true" GITHUB_ACTIONS="" scripts/core-testing.sh if [ $? -ne 0 ]; then echo "::warning::Release HA core incompatibility" - echo "true" > result.txt + FAILED="true" else echo "Successfully tested against released HA-core" - echo "false" > result.txt + FAILED="false" fi + echo "failed=$FAILED" >> "$GITHUB_OUTPUT"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/test.yml(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Setup for HA-core (release/master)
- GitHub Check: Setup for HA-core (dev)
🔇 Additional comments (3)
.github/workflows/test.yml (3)
185-193: Replace file-based result storage with step outputs.Using a file to store test results is error-prone. Consider using step outputs directly.
221-225:⚠️ Potential issueFix incorrect comment body initialization logic.
The comment body initialization logic is incorrect. It sets the message based on dev tests but then checks for release tests:
- if [[ $DEV_TESTS_FAILED == "true" ]]; then - COMMENT_BODY=":x: **Error while testing for Development HA-core:**\n\n" - else - COMMENT_BODY=":warning: **Warning while testing for RELEASED HA-core:**\n\n" - fi + COMMENT_BODY="# Test Results\n\n"Likely invalid or redundant comment.
251-266: 🛠️ Refactor suggestionAdd error handling for GitHub API calls.
The curl commands don't handle potential API failures. Add error handling:
if [[ $FAIL_COUNT -eq 1 ]]; then echo "Comment and approve the pull request" - curl -s -X POST \ + response=$(curl -s -w "%{http_code}" -X POST \ -H "Authorization: token ${{ secrets.GITHUB_TOKEN }}" \ -H "Content-Type: application/json" \ --data "{\"event\": \"APPROVE\", \"body\": \"$COMMENT_BODY\"}" \ - "https://api.github.com/repos/${{ github.repository }}/pulls/${{ github.event.pull_request.number }}/reviews" + "https://api.github.com/repos/${{ github.repository }}/pulls/${{ github.event.pull_request.number }}/reviews") + if [[ "${response: -3}" != "200" ]]; then + echo "::error::Failed to approve PR: $response" + exit 1 + fi fiLikely invalid or redundant comment.
|
✔️ Success: No problem with testing against dev HA-core. |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
.github/workflows/test.yml (2)
11-16: Improve the commented workaround code for fetching PR labels.While the commented code provides a potential solution, it could be improved using GitHub's official actions:
-# Workaround could be something like -# - name: Get PR labels -# run: | -# PR_LABELS=$(curl -s -H "Authorization: token ${{ secrets.GITHUB_TOKEN }}" \ -# "https://api.github.com/repos/${{ github.repository }}/issues/${{ github.event.pull_request.number }}/labels") -# echo "PR Labels: $PR_LABELS" +# Workaround could be: +# - name: Get PR labels +# uses: actions/github-script@v7 +# id: get-labels +# with: +# script: | +# const { data: labels } = await github.rest.issues.listLabelsOnIssue({ +# owner: context.repo.owner, +# repo: context.repo.repo, +# issue_number: context.issue.number, +# }); +# return labels.map(label => label.name);
221-225: Simplify and improve comment body construction.The initial comment body construction is inconsistent and gets overwritten.
- if [[ $DEV_TESTS_FAILED == "true" ]]; then - COMMENT_BODY=":x: **Error while testing for Development HA-core:**\n\n" - else - COMMENT_BODY=":warning: **Warning while testing for RELEASED HA-core:**\n\n" - fi + COMMENT_BODY="# Test Results\n\n"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/test.yml(4 hunks)
🔇 Additional comments (1)
.github/workflows/test.yml (1)
185-193: Apply the same test result handling improvements to the dev job.The dev job has the same test result handling issues as the release job.
Please apply the same improvements suggested for the release job to this section.
|
|
Merging as discussed |
Summary by CodeRabbit
New Features
Chores
package-lock.jsonto.gitignorepushevent trigger from the workflow configurationDocumentation
Tests
ZeroconfServiceInfoimports and variables