Conversation
Make the post-install audit TUI apply all suggested disables when selecting "Disable all" instead of only marking rows as selected. Add shared disable-application handling, improve focus navigation between the suggestion list and action buttons, and cover Disable all, Disable selected, and navigation behavior with regression tests.
Fix the post-install audit TUI so “Disable all” applies all suggested disables immediately, records the applied keys, shows the completion state, and improves focus navigation in the review screen. Restore the expected email delivery behavior: new installs default to the TIS24 relay with local sendmail failover, while manually selected PMF falls back through relay and then sendmail. Stop writing EMAIL_FALLBACK_PMF, keep it only as a transitional read alias, update installer prompts/templates/docs, and normalize the relay script version header to avoid INVALID_SCRIPT_VERSION failures from dev/local builds. Add regression coverage for the TUI action path, installer defaults, email fallback routing, config aliases, dependency checks, and relay version normalization.
Fix the post-install audit TUI so “Disable all” applies all suggested changes immediately, records applied keys, opens the completion state, and avoids review-screen focus traps. Restore email delivery defaults to relay with sendmail failover, keep PMF as an explicit manual method with relay/sendmail fallback, and normalize relay script-version headers. Fix early-error notifications by preserving ServerID, ServerMAC, and ScriptVersion when pre-backup checks fail, so relay delivery has the required identity data. Make backup/log permission checks filesystem-aware, skipping POSIX ownership warnings on CIFS/SMB/NTFS/FUSE-style filesystems, and update docs, templates, and regression tests.
Make BASE_DIR derive from the installed proxsave executable instead of backup.env or the parent environment. Add a shared base-dir resolver, load configs with the detected runtime base directory, and ignore deprecated BASE_DIR values from config/env with explicit runtime warnings. Align CLI and TUI install behavior so runtime-derived keys are removed consistently, keep upgrade/env migration from reintroducing BASE_DIR, and document the new behavior in templates and docs. Add regression coverage for resolver behavior, ignored BASE_DIR overrides, CLI/TUI config cleanup, upgrade, and legacy migration.
Use errors.Is to detect EOF in restore UI error normalization so wrapped EOF errors are treated as restore aborts. Keep the existing warning message and abort behavior unchanged, and add coverage for wrapped EOF handling.
…gger Register prepared bundle cleanup immediately after prepareBundleAndPlan so it also runs when planning returns an error after setting w.prepared. Keep cleanup guarded for nil prepared bundles and remove the internal planning cleanup path to avoid double cleanup across the shared CLI/TUI restore flow. Ensure TFA compatibility recommendations are still prompted when no logger is available. Keep logger usage limited to emitting the optional re-enrollment warning, and add coverage for confirmed and declined nil-logger flows.
Run already-registered restore service cleanups when a later service preparation step returns an error or ErrRestoreAborted. This ensures services stopped by preparePVEClusterRestore are restarted if preparePBSServices aborts, while keeping the shared CLI/TUI restore flow aligned.
Guard PBS notification API warning logs when logger is nil. Skip unknown sections and malformed endpoints without panicking, while preserving existing warning output whenever a logger is provided.
Probe mount state only once in guardMountPoint and use that result to decide whether to skip or create the guard bind mount. Return the mounted status from ensureGuardTargetUnmounted and remove the second isAlreadyMounted probe to avoid a race between mount checks.
Return and propagate errors from PVE storage subdirectory creation. Aggregate per-storage failures in RecreateStorageDirectories so partial directory recreation is reported upstream while preserving best-effort handling for remaining storage entries.
Return errors from PBS datastore subdirectory creation instead of logging and continuing. Stop initialization on the first .chunks/.index creation failure and propagate PBS datastore recreation errors upstream while preserving best-effort handling for later datastore entries.
Sync normalized PBS datastore.cfg writes before replacing the target file. Flush the temporary file before close/rename and fsync the containing directory after rename so the updated datastore.cfg entry is persisted on stable storage.
Restrict common ZFS mount path detection to mount-like paths and full path segments. The previous heuristic matched any path containing "backup" or "datastore", which could classify unrelated directories such as mybackup or datastore2 as ZFS mount points. The updated logic keeps /mnt/ handling and only treats backup/datastore as complete absolute path segments. Added focused tests for valid mount-style paths and substring false positives.
Ensure PBS datastore directory permissions are applied even when backup ownership lookup does not find the backup user.
Parse storage and datastore path lines by trimming the path prefix instead of splitting fields, preserving paths that contain spaces.
Clean up partially restored files when restore copy or close fails, logging cleanup errors and preserving existing restore flow.
Remove CR/LF from generated To and From headers to prevent MIME header injection while preserving existing sendmail and PMF flows.
Ensure OpenLogFile clears the existing logFile reference even when closing the previous file returns an error.
Escape header, storage summary, and footer values before rendering the email HTML template to prevent raw HTML injection
Use a child cancellable context for PBS PXAR worker runs so sibling datastore workers stop promptly after the first failure
Preserve compressor Start failures when tar writer pipe cleanup reports a closed-pipe error
Keep the form component test on the wrapper API and assert the password field type before checking its label
Avoid a confusing error method name in network rollback UI flow while preserving the existing logger forwarding behavior.
Extract the network apply confirmation timeout into a package-level constant next to the rollback timeout
Extract pure bridge and wireless name detection helpers and test them directly without depending on Linux sysfs state
Use a consistent multi-line style for every allowed command factory without changing the command allowlist behavior
Treat transient EIO errors from closing permission test files like create failures, retrying before reporting filesystem I/O failure
Ensure temporary symlink checks are cleaned up with a deferred removal even when inline cleanup fails
Return lock file close failures through CheckResult without also logging a duplicate warning
Clarify why the lock-file test closes during sync to exercise the subsequent production Close failure path
📝 WalkthroughWalkthroughExecutable-path base dir detection is introduced and propagated into config loading across commands. Email delivery is normalized with a sendmail-centric fallback chain and sanitized headers. Backup/orchestrator flows gain cancellation/cleanup and fsyncs. Filesystem checks skip POSIX on CIFS/FUSE. Installer/TUI and extensive tests/docs are updated. ChangesRuntime BASE_DIR detection and config/path handling
Email delivery normalization, fallback chain, installer/TUI updates
Orchestrator hardening: directory creation, restore cleanup, TUI review
Backup archiver start-failure handling and PBS collector cancellation
Security dependencies and filesystem capability-based checks
Misc fixes: identity, logging, safeexec, checks, telegram bootstrap, TUI component test
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
Not up to standards ⛔🔴 Issues
|
| Category | Results |
|---|---|
| Security | 6 critical 13 high |
| Complexity | 10 medium |
🟢 Metrics 197 complexity · 47 duplication
Metric Results Complexity 197 Duplication 47
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
internal/orchestrator/mount_guard.go (1)
66-72: ⚡ Quick winConsider renaming or removing this function for clarity.
The function name
ensureGuardTargetUnmountedimplies it takes action to ensure the target is unmounted, but it only checks the mount status without modifying anything. Since it's now just a pass-through wrapper aroundisMounted, consider either:
- Inlining it by calling
isMounteddirectly fromguardMountPoint(line 37), or- Renaming it to something like
checkMountStatusorgetMountStatusto accurately reflect its behavior.♻️ Option 1: Inline the function
func guardMountPoint(ctx context.Context, guardTarget string) error { target, err := normalizeGuardMountRequest(ctx, guardTarget) if err != nil { return err } - mounted, err := ensureGuardTargetUnmounted(target) + mounted, err := isMounted(target) if err != nil { return fmt.Errorf("check mount status: %w", err) } if mounted { return nil } guardDir := guardDirForTarget(target) if err := ensureGuardDirectories(guardDir, target); err != nil { return err } return bindReadOnlyGuard(guardDir, target) } - -func ensureGuardTargetUnmounted(target string) (bool, error) { - mounted, err := isMounted(target) - if err != nil { - return false, err - } - return mounted, nil -}🤖 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 `@internal/orchestrator/mount_guard.go` around lines 66 - 72, The function ensureGuardTargetUnmounted only forwards to isMounted but its name is misleading; remove this wrapper and update guardMountPoint to call isMounted(target) directly (preserve the existing error handling and return semantics), then delete the ensureGuardTargetUnmounted function and run tests to ensure no other references remain; alternatively, if you prefer keeping a helper, rename ensureGuardTargetUnmounted to checkMountStatus or getMountStatus and adjust its name where referenced (e.g., in guardMountPoint) to reflect that it only queries state.internal/tui/wizard/install.go (1)
456-456: 💤 Low valueMinor: Redundant runtime key removal calls.
config.RemoveRuntimeDerivedEnvKeys(template)is called at both line 456 and line 493 when the wizard is not skipped. While this is harmless (the operation is idempotent), the first call at line 456 appears unnecessary since the template will be sanitized again at line 493 right before writing.♻️ Proposed simplification
if skipConfigWizard { return installConfigResult{SkipConfigWizard: true}, nil } - template = config.RemoveRuntimeDerivedEnvKeys(template) logging.DebugStepBootstrap(bootstrap, "install config wizard (cli)", "configuring secondary storage")Also applies to: 493-493
🤖 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 `@internal/tui/wizard/install.go` at line 456, Duplicate call to config.RemoveRuntimeDerivedEnvKeys(template) is unnecessary; remove the earlier invocation (the one executed unconditionally during UI setup) and keep the single sanitization just before persisting the template inside the non-skipped path. Specifically, delete the first call to config.RemoveRuntimeDerivedEnvKeys(template) and ensure the remaining call (inside the branch that writes the template) stays so the template is sanitized right before writing.
🤖 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 `@cmd/proxsave/base_dir_test.go`:
- Around line 11-12: The test is copying a sync.Once (execInfoOnce) which is
forbidden; change the save/restore logic to use pointers instead of copying the
value: capture references (e.g., &execInfo and &execInfoOnce or store the
original pointer fields) so you don't copy the sync.Once, and update the restore
path to reassign the pointers back to execInfo and execInfoOnce (or their
underlying fields) rather than doing a value copy; ensure all uses of
origInfo/origOnce in this test use the pointer-backed originals to avoid copying
the sync.Once.
In `@docs/CONFIGURATION.md`:
- Line 868: Replace the fragment about aliases with a self-contained sentence
that explicitly states the aliases and normalization behavior; update the text
mentioning `pmf` and its aliases (`proxmox`, `proxmox-notifications`,
`proxmox-mail-forward`) so it reads as a complete sentence such as: "The value
`pmf` may also be written as `proxmox`, `proxmox-notifications`, or
`proxmox-mail-forward`; ProxSave normalizes those aliases to `pmf`." Ensure this
replaces the existing fragment mentioning `pmf`.
In `@internal/backup/archiver.go`:
- Around line 727-730: In drainTarWriterAfterCompressorStartFailure, remove the
unnecessary blank identifier by replacing the receive expression `_ = <-errChan`
with a plain receive `<-errChan>` so the function still waits for the goroutine
to finish while satisfying staticcheck S1005; keep the existing CloseWithError
on pw and use the same parameters (pw, errChan, startErr).
---
Nitpick comments:
In `@internal/orchestrator/mount_guard.go`:
- Around line 66-72: The function ensureGuardTargetUnmounted only forwards to
isMounted but its name is misleading; remove this wrapper and update
guardMountPoint to call isMounted(target) directly (preserve the existing error
handling and return semantics), then delete the ensureGuardTargetUnmounted
function and run tests to ensure no other references remain; alternatively, if
you prefer keeping a helper, rename ensureGuardTargetUnmounted to
checkMountStatus or getMountStatus and adjust its name where referenced (e.g.,
in guardMountPoint) to reflect that it only queries state.
In `@internal/tui/wizard/install.go`:
- Line 456: Duplicate call to config.RemoveRuntimeDerivedEnvKeys(template) is
unnecessary; remove the earlier invocation (the one executed unconditionally
during UI setup) and keep the single sanitization just before persisting the
template inside the non-skipped path. Specifically, delete the first call to
config.RemoveRuntimeDerivedEnvKeys(template) and ensure the remaining call
(inside the branch that writes the template) stays so the template is sanitized
right before writing.
🪄 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: 151e09ea-8715-43d4-8cfc-70f8bdc27f8d
📒 Files selected for processing (94)
cmd/proxsave/base_dir_test.gocmd/proxsave/config_helpers.gocmd/proxsave/decrypt_cmd.gocmd/proxsave/encryption_setup.gocmd/proxsave/encryption_setup_test.gocmd/proxsave/helpers_test.gocmd/proxsave/install.gocmd/proxsave/install_helpers_test.gocmd/proxsave/install_test.gocmd/proxsave/install_tui.gocmd/proxsave/main_config_modes.gocmd/proxsave/main_network.gocmd/proxsave/main_runtime.gocmd/proxsave/main_runtime_log.gocmd/proxsave/new_install.gocmd/proxsave/new_install_test.gocmd/proxsave/newkey.gocmd/proxsave/permissions.gocmd/proxsave/runtime_helpers.gocmd/proxsave/upgrade.godocs/BACKUP_ENV_MAPPING.mddocs/CLI_REFERENCE.mddocs/CONFIGURATION.mddocs/EXAMPLES.mddocs/INSTALL.mddocs/MIGRATION_GUIDE.mddocs/TROUBLESHOOTING.mdinternal/backup/archiver.gointernal/backup/archiver_test.gointernal/backup/collector_pbs_datastore.gointernal/backup/collector_pxar_datastore_test.gointernal/checks/checks.gointernal/checks/checks_test.gointernal/config/config.gointernal/config/config_test.gointernal/config/env_mutation.gointernal/config/migration.gointernal/config/migration_test.gointernal/config/templates/backup.envinternal/config/upgrade.gointernal/identity/identity.gointernal/identity/identity_test.gointernal/logging/logger.gointernal/logging/logger_test.gointernal/notify/email.gointernal/notify/email_delivery_methods_test.gointernal/notify/email_relay.gointernal/notify/email_relay_test.gointernal/notify/email_test.gointernal/notify/notify.gointernal/notify/notify_test.gointernal/notify/templates.gointernal/orchestrator/backup_safety.gointernal/orchestrator/backup_safety_test.gointernal/orchestrator/directory_recreation.gointernal/orchestrator/directory_recreation_config.gointernal/orchestrator/directory_recreation_ownership.gointernal/orchestrator/directory_recreation_paths.gointernal/orchestrator/directory_recreation_pbs.gointernal/orchestrator/directory_recreation_pbs_config.gointernal/orchestrator/directory_recreation_pve.gointernal/orchestrator/directory_recreation_test.gointernal/orchestrator/extensions.gointernal/orchestrator/extensions_additional_test.gointernal/orchestrator/mount_guard.gointernal/orchestrator/mount_guard_more_test.gointernal/orchestrator/network_apply.gointernal/orchestrator/network_apply_workflow_ui_prompt.gointernal/orchestrator/network_apply_workflow_ui_rollback.gointernal/orchestrator/notification_adapter_test.gointernal/orchestrator/pbs_notifications_api_apply.gointernal/orchestrator/pbs_notifications_api_apply_test.gointernal/orchestrator/restore_workflow_ui.gointernal/orchestrator/restore_workflow_ui_backups_services.gointernal/orchestrator/restore_workflow_ui_backups_services_test.gointernal/orchestrator/restore_workflow_ui_plan.gointernal/orchestrator/restore_workflow_ui_run.gointernal/orchestrator/restore_workflow_ui_test.gointernal/orchestrator/restore_workflow_ui_tfa.gointernal/orchestrator/restore_workflow_ui_tfa_test.gointernal/orchestrator/telegram_setup_bootstrap.gointernal/orchestrator/telegram_setup_bootstrap_test.gointernal/safeexec/safeexec.gointernal/security/security.gointernal/security/security_test.gointernal/storage/filesystem.gointernal/storage/filesystem_test.gointernal/storage/storage.gointernal/storage/storage_test.gointernal/tui/components/form_test.gointernal/tui/wizard/install.gointernal/tui/wizard/install_test.gointernal/tui/wizard/post_install_audit_tui.gointernal/tui/wizard/post_install_audit_tui_test.go
💤 Files with no reviewable changes (1)
- internal/orchestrator/restore_workflow_ui_plan.go
Comment on lines
+11
to
+12
| origInfo := execInfo | ||
| origOnce := execInfoOnce |
There was a problem hiding this comment.
Critical: sync.Once must not be copied.
Lines 12 and 26 copy sync.Once by value, which is forbidden because sync.Once contains sync.noCopy to prevent exactly this. Copying synchronization primitives can cause race conditions and undefined behavior.
🔒 Proposed fix: use pointer to avoid copying
func forceDetectedBaseDirForTest(t *testing.T, baseDir string) {
t.Helper()
origInfo := execInfo
- origOnce := execInfoOnce
+ origOnce := &execInfoOnce
execPath := filepath.Join(baseDir, "build", "proxsave")
execInfo = ExecInfo{
ExecPath: execPath,
ExecDir: filepath.Dir(execPath),
BaseDir: baseDir,
HasBase: true,
}
- execInfoOnce = sync.Once{}
+ newOnce := sync.Once{}
+ newOnce.Do(func() {})
+ execInfoOnce = newOnce
- execInfoOnce.Do(func() {})
t.Cleanup(func() {
execInfo = origInfo
- execInfoOnce = origOnce
+ execInfoOnce = *origOnce
})
}Also applies to: 26-26
🧰 Tools
🪛 golangci-lint (2.12.2)
[error] 12-12: copylocks: assignment copies lock value to origOnce: sync.Once contains sync.noCopy
(govet)
🤖 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 `@cmd/proxsave/base_dir_test.go` around lines 11 - 12, The test is copying a
sync.Once (execInfoOnce) which is forbidden; change the save/restore logic to
use pointers instead of copying the value: capture references (e.g., &execInfo
and &execInfoOnce or store the original pointer fields) so you don't copy the
sync.Once, and update the restore path to reassign the pointers back to execInfo
and execInfoOnce (or their underlying fields) rather than doing a value copy;
ensure all uses of origInfo/origOnce in this test use the pointer-backed
originals to avoid copying the sync.Once.
| | `sendmail` | The node already has a local MTA such as Postfix, Exim, or Sendmail. | In the local MTA. ProxSave calls `/usr/sbin/sendmail`. | | ||
| | `pmf` | You explicitly want Proxmox Notifications via `proxmox-mail-forward`. | In Proxmox (`Datacenter -> Notifications` on PVE, or the PBS notification UI/config). ProxSave does not ask for SMTP host/port/user/password. | | ||
|
|
||
| `pmf` may also be written as `proxmox`, `proxmox-notifications`, or `proxmox-mail-forward`; ProxSave normalizes those aliases to `pmf`. |
There was a problem hiding this comment.
Make the alias sentence explicit and self-contained.
Line 868 reads a bit fragment-like in context. Rewording it as a complete sentence improves readability in this dense section.
✍️ Suggested wording
-`pmf` may also be written as `proxmox`, `proxmox-notifications`, or `proxmox-mail-forward`; ProxSave normalizes those aliases to `pmf`.
+You can also write `pmf` as `proxmox`, `proxmox-notifications`, or `proxmox-mail-forward`; ProxSave normalizes all of these aliases to `pmf`.📝 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.
Suggested change
| `pmf` may also be written as `proxmox`, `proxmox-notifications`, or `proxmox-mail-forward`; ProxSave normalizes those aliases to `pmf`. | |
| You can also write `pmf` as `proxmox`, `proxmox-notifications`, or `proxmox-mail-forward`; ProxSave normalizes all of these aliases to `pmf`. |
🧰 Tools
🪛 LanguageTool
[style] ~868-~868: To form a complete sentence, be sure to include a subject.
Context: ... SMTP host/port/user/password. | pmf may also be written as proxmox, `proxmox-...
(MISSING_IT_THERE)
🤖 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 `@docs/CONFIGURATION.md` at line 868, Replace the fragment about aliases with a
self-contained sentence that explicitly states the aliases and normalization
behavior; update the text mentioning `pmf` and its aliases (`proxmox`,
`proxmox-notifications`, `proxmox-mail-forward`) so it reads as a complete
sentence such as: "The value `pmf` may also be written as `proxmox`,
`proxmox-notifications`, or `proxmox-mail-forward`; ProxSave normalizes those
aliases to `pmf`." Ensure this replaces the existing fragment mentioning `pmf`.
Comment on lines
+727
to
+730
| func drainTarWriterAfterCompressorStartFailure(pw *io.PipeWriter, errChan <-chan error, startErr error) { | ||
| _ = pw.CloseWithError(startErr) | ||
| _ = <-errChan | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify the non-idiomatic receive assignment is removed.
rg -nP '\b_ = <-' internal/backup/archiver.goRepository: tis24dev/proxsave
Length of output: 80
🏁 Script executed:
# Find staticcheck configuration files
fd -i 'golangci|staticcheck' -e yml -e yaml -e toml -e confRepository: tis24dev/proxsave
Length of output: 43
🏁 Script executed:
# Check context around lines 727-730 in the file
sed -n '720,735p' internal/backup/archiver.goRepository: tis24dev/proxsave
Length of output: 596
🏁 Script executed:
# Search for other instances of idiomatic receive assignment issues in the codebase
rg -n '_ = <-' --type goRepository: tis24dev/proxsave
Length of output: 108
🏁 Script executed:
# Search for CI/CD configuration files
fd -i '\.(github|gitlab)' -type dRepository: tis24dev/proxsave
Length of output: 232
🏁 Script executed:
# Check for workflow files
find . -path '*/.github/workflows/*' -type f 2>/dev/nullRepository: tis24dev/proxsave
Length of output: 348
🏁 Script executed:
# Check for Makefile or other build configuration
fd -i 'makefile|justfile|\.circleci' -type fRepository: tis24dev/proxsave
Length of output: 232
🏁 Script executed:
# Search for any reference to staticcheck in the repository
rg -i 'staticcheck' --type go --type sh --type yaml --type tomlRepository: tis24dev/proxsave
Length of output: 816
🏁 Script executed:
# Check the security-ultimate workflow
cat .github/workflows/security-ultimate.ymlRepository: tis24dev/proxsave
Length of output: 3570
🏁 Script executed:
# Check all workflow files for staticcheck references
rg 'staticcheck' .github/workflows/Repository: tis24dev/proxsave
Length of output: 248
🌐 Web query:
staticcheck S1005 Go rule definition
💡 Result:
Staticcheck S1005 is: “Drop unnecessary use of the blank identifier”. It flags assignments where the blank identifier (_) is used but isn’t needed (the identifier’s use is redundant). [1][2] In practice, it’s about cases like assigning to _ in places where Go’s language semantics make that extra blank assignment unnecessary (Staticcheck’s description is exactly that “assigning to the blank identifier is unnecessary.”) [2] To view the official online definition, use the checks page anchor for S1005: https://staticcheck.dev/docs/checks#S1005. [1][2]
Citations:
- 1: https://staticcheck.dev/docs/checks
- 2: Linter: staticcheck, Rule:
ST1005- Incorrectly formatted error string. Should we enable it? influxdata/telegraf#16818
Drop unnecessary blank identifier in receive expression.
Line 729 uses _ = <-errChan, which violates staticcheck rule S1005 (unnecessary blank identifier). Use a plain receive expression instead.
🔧 Proposed fix
func drainTarWriterAfterCompressorStartFailure(pw *io.PipeWriter, errChan <-chan error, startErr error) {
_ = pw.CloseWithError(startErr)
- _ = <-errChan
+ <-errChan
}🧰 Tools
🪛 GitHub Actions: Ultimate Security Analysis (Go) / 0_security.txt
[error] 729-729: staticcheck reported: unnecessary assignment to the blank identifier (S1005).
🪛 GitHub Actions: Ultimate Security Analysis (Go) / security
[error] 729-729: staticcheck reported: unnecessary assignment to the blank identifier (S1005)
🪛 GitHub Check: security
[failure] 729-729:
unnecessary assignment to the blank identifier (S1005)
🪛 golangci-lint (2.12.2)
[error] 729-729: S1005: unnecessary assignment to the blank identifier
(staticcheck)
🤖 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 `@internal/backup/archiver.go` around lines 727 - 730, In
drainTarWriterAfterCompressorStartFailure, remove the unnecessary blank
identifier by replacing the receive expression `_ = <-errChan` with a plain
receive `<-errChan>` so the function still waits for the goroutine to finish
while satisfying staticcheck S1005; keep the existing CloseWithError on pw and
use the same parameters (pw, errChan, startErr).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Release Notes
New Features
BASE_DIRenvironment variable is no longer user-configurable and will be ignored.Bug Fixes
Documentation