Update 8.5.6 #533
Update 8.5.6 #533
Conversation
WalkthroughThe changes in this pull request introduce a comprehensive restructuring of the UniFi Network Application add-on for Home Assistant. New functionalities for managing GitHub Actions workflows, builds, packages, pull requests, and repository interactions are implemented. The project is updated to reflect a new repository structure, including changes in documentation, CI workflows, and configuration files. Additionally, several files are deleted, and ownership and contribution information are revised. The overall organization and clarity of the codebase and documentation are enhanced. Changes
Poem
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: 34
🧹 Outside diff range and nitpick comments (20)
unifi/config.yaml (1)
19-23: LGTM: Backup exclusions are well-defined.The backup exclusions are appropriate, targeting redundant and regeneratable files:
- Backup data (already backed up)
- Firmware files (downloadable)
- System properties backup (temporary)
Consider adding a comment in the YAML to document why these specific paths are excluded:
backup_exclude: + # Exclude redundant backups and downloadable/temporary files - "*/unifi/data/backup" - "*/unifi/data/firmware" - "*/unifi/data/firmware.json" - "*/unifi/data/system.properties.bk".build/build.go (2)
13-23: Add documentation for the constructor function.Consider adding a documentation comment explaining the purpose and behavior of
newBuild().
60-73: Consider handling write errors in flagUsage.While the implementation is good, it ignores potential write errors. In a CLI context, these errors might indicate that the output is being piped to a closed pipe.
Here's a suggested improvement:
func (this *build) flagUsage(fs *flag.FlagSet, reasonMsg string, args ...any) { w := fs.Output() - _, _ = fmt.Fprint(w, `Usage of .build: + if _, err := fmt.Fprint(w, `Usage of .build: `); err != nil { + return + } if reasonMsg != "" { - _, _ = fmt.Fprintf(w, "Error: %s\n", fmt.Sprintf(reasonMsg, args...)) + if _, err := fmt.Fprintf(w, "Error: %s\n", fmt.Sprintf(reasonMsg, args...)); err != nil { + return + } } // ... similar changes for other writes ... }.build/main.go (1)
14-30: Consider improving error message specificity.The error message on line 17 could be more descriptive by indicating it's a build initialization error.
- _, _ = fmt.Fprintf(os.Stderr, "Error: %v\n", err) + _, _ = fmt.Fprintf(os.Stderr, "Build initialization error: %v\n", err)README.md (1)
Line range hint
33-47: Clean up Discord-related content.There are inconsistencies in the Discord-related content:
- The Discord badge was removed but the Discord chat server reference is still maintained in the support section
- There's an unused reference definition for "discord-shield"
Apply this diff to clean up:
- The [Home Assistant Discord chat server][discord-ha] for general Home Assistant discussions and questions. [...] - [discord-shield]: https://img.shields.io/discord/478094546522079232.svgAlso applies to: 78-78
.github/workflows/ci.yaml (2)
51-89: Improve shell script robustness and efficiencyThe shell script can be enhanced for better safety and efficiency:
function hasLabel() { - go run . has-pr-label "${1}" "${TEST_IMAGE_LABEL_NAME}" && return 0 || retVal=$? + if go run . has-pr-label "${1}" "${TEST_IMAGE_LABEL_NAME}"; then + return 0 + else + retVal=$? + fi if [ "$retVal" -eq 1 ]; then return 1 fi exit $retVal } -echo "registry=${registry}" >> "$GITHUB_OUTPUT" -echo "image=${image}" >> "$GITHUB_OUTPUT" -echo "imageTag=${imageTag}" >> "$GITHUB_OUTPUT" -echo "push=${push}" >> "$GITHUB_OUTPUT" +{ + echo "registry=${registry}" + echo "image=${image}" + echo "imageTag=${imageTag}" + echo "push=${push}" +} >> "$GITHUB_OUTPUT"🧰 Tools
🪛 actionlint
54-54: shellcheck reported issue in this script: SC2086:info:7:8: Double quote to prevent globbing and word splitting
(shellcheck)
54-54: shellcheck reported issue in this script: SC2129:style:26:1: Consider using { cmd1; cmd2; } >> file instead of individual redirects
(shellcheck)
54-54: shellcheck reported issue in this script: SC2129:style:31:1: Consider using { cmd1; cmd2; } >> file instead of individual redirects
(shellcheck)
🪛 yamllint
[error] 63-63: trailing spaces
(trailing-spaces)
158-165: Consider adjusting artifact retention periodThe current retention period of 1 day for digests might be too short if there are delays in the workflow or if manual intervention is needed.
Consider increasing the retention period:
with: name: digests-${{ steps.refs.outputs.platformPair }} path: /tmp/digests/* if-no-files-found: error - retention-days: 1 + retention-days: 5.build/actions.go (1)
26-26: Implement validation logic in the 'Validate' methodThe
Validatemethod currently returnsniland does not perform any validation. If validation is needed for theactionsstruct, consider implementing the necessary logic. Otherwise, you might remove the method if it's not required..build/packages.go (3)
18-114: Use idiomatic receiver names in methodsThe receiver name
thisis not idiomatic in Go. Conventionally, receiver names should be short, usually one or two letters, often derived from the type name. Consider renamingthistoporpkgfor thepackagestype to enhance readability and adhere to Go conventions.Apply this diff to update the receiver names:
-func (this *packages) init(b *build, fs *flag.FlagSet) { +func (p *packages) init(b *build, fs *flag.FlagSet) { this.build = b fs.Var(&this.subs, "package-subs", "") - b.registerCommand("delete-image-tag", "<tag> [<tag> ..]", this.deleteVersionsWithTags) + b.registerCommand("delete-image-tag", "<tag> [<tag> ..]", p.deleteVersionsWithTags) } -func (this *packages) Validate() error { return nil } +func (p *packages) Validate() error { return nil } // ... and similarly update all methods within the `packages` type.
116-139: Use idiomatic receiver names inpackageVersionmethodsFor the
packageVersiontype, consider renaming the receiverthistopvorvto follow Go naming conventions.Apply this diff to update the receiver names:
-func (this *packageVersion) delete(ctx context.Context) error { +func (pv *packageVersion) delete(ctx context.Context) error { var m func(context.Context, string, string, string, int64) (*github.Response, error) - if this.parent.build.ownerType == user { - m = this.parent.build.client.Users.PackageDeleteVersion + if pv.parent.build.ownerType == user { + m = pv.parent.build.client.Users.PackageDeleteVersion } else { - m = this.parent.build.client.Organizations.PackageDeleteVersion + m = pv.parent.build.client.Organizations.PackageDeleteVersion } - if _, err := m(ctx, this.parent.build.owner.String(), "container", this.parent.build.repo.SubName(this.sub), *this.ID); err != nil { - return fmt.Errorf("cannot delete package version %v: %w", this, err) + if _, err := m(ctx, pv.parent.build.owner.String(), "container", pv.parent.build.repo.SubName(pv.sub), *pv.ID); err != nil { + return fmt.Errorf("cannot delete package version %v: %w", pv, err) } return nil } -func (this packageVersion) String() string { - return fmt.Sprintf("%s(%d)@%s", *this.Name, *this.ID, this.parent.build.SubString(this.sub)) +func (pv packageVersion) String() string { + return fmt.Sprintf("%s(%d)@%s", *pv.Name, *pv.ID, pv.parent.build.SubString(pv.sub)) }
43-47: Use consistent logging and ensure message formattingThe success message uses
fmt.Printfwithout a newline character, which may cause concatenation with subsequent output. Additionally, to maintain consistent logging practices, consider usinglog.Printfinstead offmt.Printf.Apply this diff to update the logging:
if err := candidate.delete(ctx); err != nil { log.Println("WARN", err) } else { - fmt.Printf("INFO successfully deleted package version %v", candidate) + log.Printf("INFO successfully deleted package version %v", candidate) }.build/prs.go (4)
100-100: Remove unreachable code afteros.Exitcalls.The
return nilstatements after theos.Exitcalls are unreachable and can be removed to clean up the code.Apply this diff:
- return nilAlso applies to: 122-122
66-66: Consider logging meaningful information instead ofthis.In the log message on line 66,
thisrefers to theprsstruct, which may not provide useful information. It would be better to log the workflow run or relevant details.Apply this diff to improve the log message:
- log.Printf("INFO latest workflow run %v is still running (waiting since %v)...", this, time.Since(start).Truncate(time.Second)) + log.Printf("INFO latest workflow run %v is still running (waiting since %v)...", wfr, time.Since(start).Truncate(time.Second))
35-41: Simplify argument length checks by consolidating conditions.You can improve the readability by checking all required arguments in a single condition.
Apply this diff:
func (this *prs) rerunLatestWorkflowCmd(ctx context.Context, args []string) error { - if len(args) < 1 { - return flagFail("no pr number provided") - } - if len(args) < 2 { - return flagFail("no workflow filename provided") + if len(args) < 2 { + return flagFail("usage: rerun-pr-workflow <prNumber> <workflowFilename>") } // ... existing code ... }
20-20: Rename receiver fromthisto comply with Go naming conventions.In Go, the receiver name should be of the form
prorprsinstead ofthisto follow standard conventions.Apply this diff:
-func (this *prs) init(b *build, _ *flag.FlagSet) { - this.build = b +func (prs *prs) init(b *build, _ *flag.FlagSet) { + prs.build = b // ... existing code ... }Repeat this change for all methods with the receiver named
this..build/repo.go (5)
47-55: Use conventional receiver names instead ofthisIn the methods of the
repostruct, the receiver is namedthis. According to Go conventions, receiver names should be short and reflect the type name, such asrforrepo.Consider renaming the receiver
thistorto align with Go idioms and improve code readability.
111-143: Adopt conventional receiver namesThe methods for
ownerTypeusethisas the receiver name. In Go, it is customary to use short receiver names related to the type, such asotforownerType.Consider renaming the receiver
thistootfor clarity and consistency with Go conventions.
152-165: Adopt conventional receiver namesThe methods for
ownerusethisas the receiver name. It's conventional in Go to use short receiver names likeoforowner.Consider renaming the receiver
thistoofor improved readability.
179-192: Adopt conventional receiver namesThe methods for
repoNameusethisas the receiver name. Using a shorter receiver name likernforrepoNamefollows Go conventions and enhances code readability.Consider renaming the receiver
thistornfor consistency.
19-19: Standardize error message formatting according to Go conventionsError messages should start with lowercase letters (unless beginning with proper nouns) and should not end with punctuation, following Go conventions. Some error messages in the code do not adhere to this standard. For example:
- Line 25: "GITHUB_REPOSITORY: illegal github repository: %s" (should be "illegal GitHub repository")
- Line 172: "illegal owner: %s"
Consider revising the error messages to ensure they start with lowercase letters (except for proper nouns like "GitHub") and avoid ending punctuation, to maintain consistency and follow Go best practices.
Also applies to: 25-25, 28-28, 31-31, 36-36, 41-41, 130-130, 169-169, 172-172, 196-196, 199-199
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
.build/go.sumis excluded by!**/*.sum
📒 Files selected for processing (34)
.build/actions.go(1 hunks).build/build.go(1 hunks).build/go.mod(1 hunks).build/main.go(1 hunks).build/packages.go(1 hunks).build/prs.go(1 hunks).build/repo.go(1 hunks).github/CODEOWNERS(0 hunks).github/CONTRIBUTING.md(1 hunks).github/FUNDING.yml(0 hunks).github/workflows/ci.yaml(1 hunks).github/workflows/deploy.yaml(0 hunks).github/workflows/labels.yaml(0 hunks).github/workflows/lock.yaml(0 hunks).github/workflows/pr-images.yaml(1 hunks).github/workflows/pr-labels.yaml(0 hunks).github/workflows/release-drafter.yaml(0 hunks).github/workflows/stale.yaml(0 hunks).gitignore(1 hunks).idea/.gitignore(1 hunks).mdlrc(0 hunks).yamllint(0 hunks)LICENSE(1 hunks)README.md(2 hunks)repository.yaml(1 hunks)unifi/.README.j2(3 hunks)unifi/DOCS.md(3 hunks)unifi/Dockerfile(2 hunks)unifi/build.yaml(0 hunks)unifi/config.yaml(1 hunks)unifi/rootfs/etc/s6-overlay/s6-rc.d/init-unifi/run(1 hunks)unifi/rootfs/etc/s6-overlay/s6-rc.d/unifi/finish(1 hunks)unifi/rootfs/etc/s6-overlay/s6-rc.d/unifi/run(1 hunks)unifi/translations/de.yaml(1 hunks)
💤 Files with no reviewable changes (11)
- .github/CODEOWNERS
- .github/FUNDING.yml
- .github/workflows/deploy.yaml
- .github/workflows/labels.yaml
- .github/workflows/lock.yaml
- .github/workflows/pr-labels.yaml
- .github/workflows/release-drafter.yaml
- .github/workflows/stale.yaml
- .mdlrc
- .yamllint
- unifi/build.yaml
✅ Files skipped from review due to trivial changes (10)
- .build/go.mod
- .github/CONTRIBUTING.md
- .gitignore
- .idea/.gitignore
- LICENSE
- repository.yaml
- unifi/rootfs/etc/s6-overlay/s6-rc.d/init-unifi/run
- unifi/rootfs/etc/s6-overlay/s6-rc.d/unifi/finish
- unifi/rootfs/etc/s6-overlay/s6-rc.d/unifi/run
- unifi/translations/de.yaml
🧰 Additional context used
🪛 actionlint
.github/workflows/ci.yaml
54-54: shellcheck reported issue in this script: SC2086:info:7:8: Double quote to prevent globbing and word splitting
(shellcheck)
54-54: shellcheck reported issue in this script: SC2129:style:26:1: Consider using { cmd1; cmd2; } >> file instead of individual redirects
(shellcheck)
54-54: shellcheck reported issue in this script: SC2129:style:31:1: Consider using { cmd1; cmd2; } >> file instead of individual redirects
(shellcheck)
205-205: shellcheck reported issue in this script: SC2046:warning:1:33: Quote this to prevent word splitting
(shellcheck)
205-205: shellcheck reported issue in this script: SC2046:warning:2:3: Quote this to prevent word splitting
(shellcheck)
.github/workflows/pr-images.yaml
51-51: shellcheck reported issue in this script: SC2086:info:8:8: Double quote to prevent globbing and word splitting
(shellcheck)
51-51: shellcheck reported issue in this script: SC2086:info:16:8: Double quote to prevent globbing and word splitting
(shellcheck)
🪛 yamllint
.github/workflows/ci.yaml
[error] 63-63: trailing spaces
(trailing-spaces)
[error] 116-116: trailing spaces
(trailing-spaces)
[error] 207-207: trailing spaces
(trailing-spaces)
[error] 211-211: trailing spaces
(trailing-spaces)
.github/workflows/pr-images.yaml
[error] 53-53: trailing spaces
(trailing-spaces)
[error] 61-61: trailing spaces
(trailing-spaces)
[error] 69-69: trailing spaces
(trailing-spaces)
[error] 73-73: trailing spaces
(trailing-spaces)
[error] 77-77: trailing spaces
(trailing-spaces)
[error] 82-82: trailing spaces
(trailing-spaces)
[error] 87-87: trailing spaces
(trailing-spaces)
[error] 92-92: trailing spaces
(trailing-spaces)
🪛 Markdownlint
README.md
78-78: Unused link or image reference definition: "discord-shield"
Link and image reference definitions should be needed
(MD053, link-image-reference-definitions)
unifi/DOCS.md
169-169: Unused link or image reference definition: "discord"
Link and image reference definitions should be needed
(MD053, link-image-reference-definitions)
🔇 Additional comments (21)
unifi/Dockerfile (1)
1-1: Consider pinning critical package versions for stability.
The removal of version constraints for critical packages like mongodb-server and openjdk-17-jre-headless could lead to unexpected behavior if incompatible versions are installed. This is particularly important for MongoDB, as different versions might have compatibility issues with UniFi.
Let's verify the MongoDB version compatibility with UniFi 8.5.6:
Also applies to: 10-10
unifi/config.yaml (3)
10-12: LGTM: Ingress configuration is properly configured.
The ingress configuration is well-structured:
- Uses the correct HTTPS portal redirection port (8843)
- Enables streaming support for real-time data
3-3: Verify version and repository references across the project.
The repository ownership transfer and version update need to be consistently reflected across the project.
#!/bin/bash
# Description: Verify version and repository references are consistent
# Check version references
echo "Checking version references..."
rg "version:|VERSION=|version =|\"version\":" -g "!unifi/config.yaml"
# Check repository references
echo "Checking repository references..."
rg "hassio-addons/addon-unifi" --type yaml --type markdownAlso applies to: 6-7
14-16: Verify build support for armv7 architecture.
The addition of armv7 architecture support is beneficial for broader device compatibility.
unifi/.README.j2 (1)
Line range hint 1-15: Verify screenshot path and branding change.
The removal of "Community" from the title suggests a status change. Please confirm if this is intentional and aligns with the project's current status.
Let's verify the screenshot path:
✅ Verification successful
Branding change and screenshot path are verified ✓
The screenshot path is valid as confirmed by the presence of images/screenshot.jpg in the repository. The branding change from "Community Add-on" to "Add-on" appears to be part of an intentional update, and since this is a template file (.j2), the change will maintain consistency in the generated documentation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if the screenshot exists in the expected location
fd "screenshot.jpg" images/
Length of output: 49
.build/build.go (4)
1-11: LGTM! Well-structured imports and package declaration.
The code uses appropriate imports and a specific version of the GitHub client, which is a good practice for dependency management.
49-54: LGTM! Clean validation implementation.
The validation logic is well-structured and properly handles errors.
25-38: Add documentation for struct fields.
Consider adding documentation comments for the struct fields to improve code maintainability.
Verify the repo type implementation.
The repo type is embedded but not defined in this file.
Let's verify the repo type implementation:
#!/bin/bash
# Search for repo type definition
ast-grep --pattern 'type repo struct {
$$$
}'20-20: Consider making the wait timeout configurable.
A default timeout of 3 seconds might be too short for some GitHub API operations, especially in environments with slower network connections. Consider:
- Making it configurable via environment variable
- Increasing the default value to something more conservative (e.g., 10 seconds)
Let's check if there are any GitHub API calls in the codebase that might be affected by this timeout:
.build/main.go (2)
3-12: LGTM: Clean and well-organized imports.
All imports are from the standard library and are necessary for the functionality.
52-53: 🛠️ Refactor suggestion
Verify context state before command execution.
Consider checking if the context is already cancelled before executing the command to prevent unnecessary work.
- return cmd.f(ctx, args[1:])
+ if err := ctx.Err(); err != nil {
+ return fmt.Errorf("command cancelled before execution: %w", err)
+ }
+ return cmd.f(ctx, args[1:]).github/workflows/pr-images.yaml (3)
17-23: LGTM! Well-defined trigger events.
The workflow triggers are correctly configured for the intended PR label management use cases.
24-47: LGTM! Well-structured job configuration.
The job configuration follows best practices:
- Minimal required permissions
- Proper Go setup with version file
- Efficient dependency caching
71-71: Verify Go commands functionality.
The workflow relies on several custom Go commands. Let's verify their existence and implementation.
Also applies to: 75-75
README.md (4)
Line range hint 1-15: LGTM! Title and badges are properly updated.
The changes to the title and badges section accurately reflect the repository transfer and rebranding.
Line range hint 17-31: LGTM! Core documentation preserved.
The essential add-on description and functionality details are maintained appropriately.
Line range hint 49-63: LGTM! Attribution and contributing guidelines properly maintained.
The section correctly maintains attribution while updating repository-specific links.
74-93: Verify repository links accessibility.
While the links have been consistently updated to the new repository, let's verify they are accessible.
✅ Verification successful
Repository links are correctly configured and accessible
All repository links have been properly updated to point to the new blaubaer/ha-addon-unifi repository. Verification confirms:
- Main repository is accessible
- Documentation file exists at the specified path
- All shield URLs and repository references are consistently updated
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the accessibility of repository links
# Test: Check if the main repository URL is accessible
gh repo view blaubaer/ha-addon-unifi --json url >/dev/null 2>&1
if [ $? -eq 0 ]; then
echo "✓ Repository is accessible"
else
echo "✗ Repository might not be accessible or public"
fi
# Test: Check if the documentation file exists
gh api /repos/blaubaer/ha-addon-unifi/contents/unifi/DOCS.md >/dev/null 2>&1
if [ $? -eq 0 ]; then
echo "✓ Documentation file exists"
else
echo "✗ Documentation file might be missing or inaccessible"
fi
Length of output: 276
🧰 Tools
🪛 Markdownlint
78-78: Unused link or image reference definition: "discord-shield"
Link and image reference definitions should be needed
(MD053, link-image-reference-definitions)
unifi/DOCS.md (1)
Line range hint 1-29: LGTM! Installation steps are well-structured.
The installation steps are clear, logically ordered, and provide comprehensive guidance for setting up the UniFi Network Application.
.github/workflows/ci.yaml (1)
1-240: Overall workflow structure looks good!
The workflow is well-organized with proper job dependencies, scoped permissions, and concurrency handling. It effectively manages different scenarios (PR, push, release) and includes appropriate checks and balances.
🧰 Tools
🪛 actionlint
54-54: shellcheck reported issue in this script: SC2086:info:7:8: Double quote to prevent globbing and word splitting
(shellcheck)
54-54: shellcheck reported issue in this script: SC2129:style:26:1: Consider using { cmd1; cmd2; } >> file instead of individual redirects
(shellcheck)
54-54: shellcheck reported issue in this script: SC2129:style:31:1: Consider using { cmd1; cmd2; } >> file instead of individual redirects
(shellcheck)
205-205: shellcheck reported issue in this script: SC2046:warning:1:33: Quote this to prevent word splitting
(shellcheck)
205-205: shellcheck reported issue in this script: SC2046:warning:2:3: Quote this to prevent word splitting
(shellcheck)
🪛 yamllint
[error] 63-63: trailing spaces
(trailing-spaces)
[error] 116-116: trailing spaces
(trailing-spaces)
[error] 207-207: trailing spaces
(trailing-spaces)
[error] 211-211: trailing spaces
(trailing-spaces)
.build/prs.go (1)
52-73: Ensure proper timeout handling in the loop to prevent infinite waits.
Verify that this.build.waitTimeout is set appropriately. If it's zero or not initialized, the time.After could cause the loop to spin without delay, leading to high CPU usage.
Run the following script to check the value of waitTimeout:
✅ Verification successful
No timeout handling issues found - the configuration is properly initialized
The waitTimeout field is properly initialized in the newBuild() function with a default value of 3 seconds (time.Second * 3). Additionally, it can be configured via command-line flags using -wait-timeout. The timeout in the loop is working as intended and won't cause infinite spins or high CPU usage.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check where 'waitTimeout' is set in the codebase.
# Search for the initialization of 'waitTimeout' in the 'build' struct.
rg --type go 'waitTimeout' --context 5
Length of output: 187
Script:
#!/bin/bash
# Let's try a more comprehensive search for the struct and timeout-related fields
ast-grep --pattern 'type $_ struct {
$$$
waitTimeout $_
$$$
}'
# Also search for any timeout-related configuration
rg -g '*.go' -i 'timeout'
Length of output: 245
Script:
#!/bin/bash
# Let's first find all Go files in the repository
fd -e go
# Then let's examine the content of the file containing the code snippet
cat .build/prs.go
Length of output: 4004
Script:
#!/bin/bash
# Let's examine the build struct definition and initialization
cat .build/build.go
Length of output: 1516
| \ | ||
| && curl -J -L -o /tmp/unifi.deb \ | ||
| "https://dl.ui.com/unifi/8.3.32/unifi_sysvinit_all.deb" \ | ||
| && curl -sSLf "https://dl.ui.com/unifi/8.5.6/unifi_sysvinit_all.deb" > /tmp/unifi.deb \ |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add checksum verification for the UniFi package download.
While the download URL and curl flags are secure, adding SHA256 checksum verification would enhance security by ensuring package integrity.
Consider adding checksum verification:
+ARG UNIFI_DEB_SHA256="<official-sha256-checksum>"
RUN \
apt-get update \
&& apt-get upgrade -y \
&& apt-get install -y --no-install-recommends binutils libcap2 logrotate mongodb-server openjdk-17-jre-headless \
\
&& curl -sSLf "https://dl.ui.com/unifi/8.5.6/unifi_sysvinit_all.deb" > /tmp/unifi.deb \
+ && echo "${UNIFI_DEB_SHA256} /tmp/unifi.deb" | sha256sum -c - \
\
&& dpkg --install /tmp/unifi.deb \Committable suggestion skipped: line range outside the PR's diff.
| @@ -59,20 +51,9 @@ This repository was created for: | |||
|
|
|||
| If you are more interested in stable releases of our add-ons: | |||
|
|
|||
| <https://github.com/hassio-addons/repository> | |||
| <https://github.com/blaubaer/ha-addon-unifi> | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use {{ repo }} variable for consistency.
The beta section uses a hardcoded URL while the edge section uses the {{ repo }} variable. Consider using the variable here for consistency and easier maintenance.
-<https://github.com/blaubaer/ha-addon-unifi>
+<{{ repo }}>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <https://github.com/blaubaer/ha-addon-unifi> | |
| <{{ repo }}> |
| [release]: {{ repo }}/tree/{{ version }} | ||
| [screenshot]: https://github.com/hassio-addons/addon-unifi/raw/main/images/screenshot.jpg | ||
| [screenshot]: {{ repo }}/raw/mainmain/images/screenshot.jpg |
There was a problem hiding this comment.
Fix duplicated 'main' in screenshot path.
The screenshot URL contains a duplicated "main" in the path.
-[screenshot]: {{ repo }}/raw/mainmain/images/screenshot.jpg
+[screenshot]: {{ repo }}/raw/main/images/screenshot.jpg📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| [screenshot]: {{ repo }}/raw/mainmain/images/screenshot.jpg | |
| [screenshot]: {{ repo }}/raw/main/images/screenshot.jpg |
| func (this *build) registerCommand(name, usage string, action func(context.Context, []string) error) { | ||
| this.commands[name] = command{action, usage} | ||
| } |
There was a problem hiding this comment.
Add input validation for command registration.
The current implementation lacks validation for:
- Empty command name
- Nil action function
- Duplicate command names
Consider adding these checks to prevent runtime issues.
Here's a suggested improvement:
func (this *build) registerCommand(name, usage string, action func(context.Context, []string) error) {
+ if name == "" {
+ panic("command name cannot be empty")
+ }
+ if action == nil {
+ panic("command action cannot be nil")
+ }
+ if _, exists := this.commands[name]; exists {
+ panic(fmt.Sprintf("command %q already registered", name))
+ }
this.commands[name] = command{action, usage}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func (this *build) registerCommand(name, usage string, action func(context.Context, []string) error) { | |
| this.commands[name] = command{action, usage} | |
| } | |
| func (this *build) registerCommand(name, usage string, action func(context.Context, []string) error) { | |
| if name == "" { | |
| panic("command name cannot be empty") | |
| } | |
| if action == nil { | |
| panic("command action cannot be nil") | |
| } | |
| if _, exists := this.commands[name]; exists { | |
| panic(fmt.Sprintf("command %q already registered", name)) | |
| } | |
| this.commands[name] = command{action, usage} | |
| } |
| func (this *build) init(fs *flag.FlagSet) { | ||
| this.client = github.NewClient(nil). | ||
| WithAuthToken(os.Getenv("GITHUB_TOKEN")) | ||
|
|
||
| fs.DurationVar(&this.waitTimeout, "wait-timeout", this.waitTimeout, "") | ||
|
|
||
| this.repo.init(this, fs) | ||
| } |
There was a problem hiding this comment.
Add GitHub token validation.
The code doesn't validate if the GitHub token is present or valid. Consider:
- Checking if the token is empty
- Validating the token by making a test API call
- Returning an error if authentication fails
Here's a suggested improvement:
func (this *build) init(fs *flag.FlagSet) {
+ token := os.Getenv("GITHUB_TOKEN")
+ if token == "" {
+ return fmt.Errorf("GITHUB_TOKEN environment variable is not set")
+ }
this.client = github.NewClient(nil).
- WithAuthToken(os.Getenv("GITHUB_TOKEN"))
+ WithAuthToken(token)
+
+ // Validate token by making a test API call
+ ctx := context.Background()
+ _, _, err := this.client.Users.Get(ctx, "")
+ if err != nil {
+ return fmt.Errorf("invalid GitHub token: %v", err)
+ }Committable suggestion skipped: line range outside the PR's diff.
|
|
||
| func (this *pr) hasLabel(label string) bool { | ||
| return slices.ContainsFunc(this.Labels, func(candidate *github.Label) bool { | ||
| if candidate == nil { | ||
| return false | ||
| } | ||
| return candidate.Name != nil && *candidate.Name == label | ||
| }) |
There was a problem hiding this comment.
Handle potential nil pointers when checking for labels.
In the hasLabel function, ensure that the code safely handles potential nil pointers to prevent panics.
Consider adding checks or using safer pointer dereferencing.
func (this *pr) hasLabel(label string) bool {
return slices.ContainsFunc(this.Labels, func(candidate *github.Label) bool {
if candidate == nil || candidate.Name == nil {
return false
}
- return *candidate.Name == label
+ return strings.EqualFold(*candidate.Name, label)
})
}Committable suggestion skipped: line range outside the PR's diff.
|
|
||
| func (this *pr) latestWorkflowRun(ctx context.Context, workflowFn string) (*workflowRun, error) { | ||
| wf, err := this.parent.build.actions.workflowByFilename(ctx, workflowFn) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("cannot get workflow %s of %v: %w", workflowFn, this, err) | ||
| } | ||
| for candidate, err := range wf.runs(ctx) { | ||
| if err != nil { | ||
| return nil, fmt.Errorf("cannot retrieve workflow runs for pr %v: %w", this, err) | ||
| } | ||
| if slices.ContainsFunc(candidate.PullRequests, func(cpr *github.PullRequest) bool { | ||
| return cpr != nil && cpr.ID != nil && *cpr.ID == *this.ID | ||
| }) { | ||
| return candidate, nil | ||
| } | ||
| } | ||
| return nil, nil |
There was a problem hiding this comment.
Handle the case where no workflow run is found for the PR.
The latestWorkflowRun function may return nil if no matching workflow run is found, which could lead to a nil pointer dereference later.
Add error handling for the scenario when no workflow run is found.
func (this *pr) latestWorkflowRun(ctx context.Context, workflowFn string) (*workflowRun, error) {
// ... existing code ...
}
}
- return nil, nil
+ return nil, fmt.Errorf("no workflow run found for PR #%d with workflow '%s'", *this.Number, workflowFn)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func (this *pr) latestWorkflowRun(ctx context.Context, workflowFn string) (*workflowRun, error) { | |
| wf, err := this.parent.build.actions.workflowByFilename(ctx, workflowFn) | |
| if err != nil { | |
| return nil, fmt.Errorf("cannot get workflow %s of %v: %w", workflowFn, this, err) | |
| } | |
| for candidate, err := range wf.runs(ctx) { | |
| if err != nil { | |
| return nil, fmt.Errorf("cannot retrieve workflow runs for pr %v: %w", this, err) | |
| } | |
| if slices.ContainsFunc(candidate.PullRequests, func(cpr *github.PullRequest) bool { | |
| return cpr != nil && cpr.ID != nil && *cpr.ID == *this.ID | |
| }) { | |
| return candidate, nil | |
| } | |
| } | |
| return nil, nil | |
| func (this *pr) latestWorkflowRun(ctx context.Context, workflowFn string) (*workflowRun, error) { | |
| wf, err := this.parent.build.actions.workflowByFilename(ctx, workflowFn) | |
| if err != nil { | |
| return nil, fmt.Errorf("cannot get workflow %s of %v: %w", workflowFn, this, err) | |
| } | |
| for candidate, err := range wf.runs(ctx) { | |
| if err != nil { | |
| return nil, fmt.Errorf("cannot retrieve workflow runs for pr %v: %w", this, err) | |
| } | |
| if slices.ContainsFunc(candidate.PullRequests, func(cpr *github.PullRequest) bool { | |
| return cpr != nil && cpr.ID != nil && *cpr.ID == *this.ID | |
| }) { | |
| return candidate, nil | |
| } | |
| } | |
| return nil, fmt.Errorf("no workflow run found for PR #%d with workflow '%s'", *this.Number, workflowFn) |
| func newRepo() (repo, error) { | ||
| result := repo{ | ||
| packages: newPackages(), | ||
| prs: newPrs(), | ||
| actions: newActions(), | ||
| } | ||
| if v, ok := os.LookupEnv("GITHUB_OWNER_TYPE"); ok { | ||
| if err := result.ownerType.Set(v); err != nil { | ||
| return repo{}, fmt.Errorf("GITHUB_OWNER_TYPE: %w", err) | ||
| } | ||
| } | ||
| if v, ok := os.LookupEnv("GITHUB_REPOSITORY"); ok { | ||
| parts := strings.Split(v, "/") | ||
| if len(parts) != 2 { | ||
| return repo{}, fmt.Errorf("GITHUB_REPOSITORY: illegal github repository: %s", v) | ||
| } | ||
| if err := result.owner.Set(parts[0]); err != nil { | ||
| return repo{}, fmt.Errorf("GITHUB_REPOSITORY: %w", err) | ||
| } | ||
| if err := result.name.Set(parts[1]); err != nil { | ||
| return repo{}, fmt.Errorf("GITHUB_REPOSITORY: %w", err) | ||
| } | ||
| } | ||
| if v, ok := os.LookupEnv("GITHUB_REPOSITORY_OWNER"); ok { | ||
| if err := result.owner.Set(v); err != nil { | ||
| return repo{}, fmt.Errorf("GITHUB_REPOSITORY_OWNER: %w", err) | ||
| } | ||
| } | ||
| if v, ok := os.LookupEnv("GITHUB_REPO"); ok { | ||
| if err := result.name.Set(v); err != nil { | ||
| return repo{}, fmt.Errorf("GITHUB_REPO: %w", err) | ||
| } | ||
| } | ||
| return result, nil | ||
| } |
There was a problem hiding this comment.
Potential conflict when setting owner and repository name
The newRepo function may overwrite the owner and name fields if both GITHUB_REPOSITORY and GITHUB_REPOSITORY_OWNER/GITHUB_REPO environment variables are set. This could lead to inconsistent or unintended values being used.
Consider adding logic to handle cases where both sets of environment variables are present, possibly by prioritizing one over the other or by reporting an error if inconsistent values are detected.
| func (this repo) SubName(sub string) string { | ||
| if sub == "" { | ||
| return this.name.String() | ||
| } | ||
| return fmt.Sprintf("%v%%2F%s", this.name, sub) | ||
| } | ||
|
|
||
| func (this repo) SubString(sub string) string { | ||
| if sub == "" { | ||
| return this.String() | ||
| } | ||
| return fmt.Sprintf("%v/%s", this, sub) | ||
| } |
There was a problem hiding this comment.
Validate or sanitize sub parameter to prevent potential vulnerabilities
The methods SubName and SubString use the sub parameter directly in string formatting without validation or sanitization. If sub can contain special characters or comes from external sources, this may lead to unexpected behavior or security issues.
Consider validating the sub parameter or ensuring it is properly encoded if used in URLs or file paths.
| var ownerRegex = regexp.MustCompile("^[a-zA-Z0-9](?:[a-zA-Z0-9-]*[a-zA-Z0-9])?$") | ||
|
|
||
| func (this *owner) Set(v string) error { | ||
| buf := owner(v) | ||
| if err := buf.Validate(); err != nil { | ||
| return err | ||
| } | ||
| *this = buf | ||
| return nil | ||
| } | ||
|
|
||
| func (this owner) Validate() error { | ||
| if this == "" { | ||
| return fmt.Errorf("no owner provided") | ||
| } | ||
| if !ownerRegex.MatchString(string(this)) { | ||
| return fmt.Errorf("illegal owner: %s", this) | ||
| } | ||
| return nil | ||
| } |
There was a problem hiding this comment.
ownerRegex may not accept all valid GitHub usernames
The regular expression ownerRegex used to validate the owner may be too restrictive. GitHub usernames can include underscores (_), but the current regex does not allow them.
Update the ownerRegex to include underscores to accurately validate GitHub usernames.
Suggested change:
-var ownerRegex = regexp.MustCompile("^[a-zA-Z0-9](?:[a-zA-Z0-9-]*[a-zA-Z0-9])?$")
+var ownerRegex = regexp.MustCompile("^[a-zA-Z0-9](?:[a-zA-Z0-9-_]*[a-zA-Z0-9])?$")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| var ownerRegex = regexp.MustCompile("^[a-zA-Z0-9](?:[a-zA-Z0-9-]*[a-zA-Z0-9])?$") | |
| func (this *owner) Set(v string) error { | |
| buf := owner(v) | |
| if err := buf.Validate(); err != nil { | |
| return err | |
| } | |
| *this = buf | |
| return nil | |
| } | |
| func (this owner) Validate() error { | |
| if this == "" { | |
| return fmt.Errorf("no owner provided") | |
| } | |
| if !ownerRegex.MatchString(string(this)) { | |
| return fmt.Errorf("illegal owner: %s", this) | |
| } | |
| return nil | |
| } | |
| var ownerRegex = regexp.MustCompile("^[a-zA-Z0-9](?:[a-zA-Z0-9-_]*[a-zA-Z0-9])?$") | |
| func (this *owner) Set(v string) error { | |
| buf := owner(v) | |
| if err := buf.Validate(); err != nil { | |
| return err | |
| } | |
| *this = buf | |
| return nil | |
| } | |
| func (this owner) Validate() error { | |
| if this == "" { | |
| return fmt.Errorf("no owner provided") | |
| } | |
| if !ownerRegex.MatchString(string(this)) { | |
| return fmt.Errorf("illegal owner: %s", this) | |
| } | |
| return nil | |
| } |
Summary by CodeRabbit
Release Notes
New Features
Improvements
Bug Fixes
Chores