fix(pkg): address ignored errors with logging and documentation by ArangoGutierrez · Pull Request #610 · NVIDIA/holodeck

ArangoGutierrez · 2026-02-04T16:23:19Z

Summary

Address 28 instances of ignored errors (//nolint:errcheck) in pkg/ by either:

Logging errors that indicate state tracking issues
Documenting why cleanup errors are safe to ignore

Changes

pkg/provider/aws/create.go (12 errors fixed)

Status update functions were silently ignoring errors. Now they log warnings:

// Before
p.updateProgressingCondition(...) // nolint:errcheck

// After
if err := p.updateProgressingCondition(...); err != nil {
    p.log.Warning("Failed to update progressing condition: %v", err)
}

pkg/provisioner/provisioner.go (13 errors documented)

Close() errors in cleanup paths are now documented:

// Before
defer session.Close() // nolint:errcheck

// After  
defer session.Close() // Ignore: cleanup error - resource is being cleaned up anyway

pkg/utils/kubeconfig.go & ip.go (3 errors documented)

Added explanatory comments for cleanup close operations.

Impact

Category	Before	After
Status update errors	Silently ignored	Logged as warnings
Close() errors	`nolint:errcheck`	Documented with explanation
Observability	Poor	Improved

Test plan

go build ./pkg/... - compiles successfully
go test ./pkg/... - verify no regressions

coveralls · 2026-02-04T16:26:06Z

Pull Request Test Coverage Report for Build 21755908155

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

For more information on this, see Tracking coverage changes with pull request builds.
To avoid this issue with future PRs, see these Recommended CI Configurations.
For a quick fix, rebase this PR at GitHub. Your next report should be accurate.

Details

13 of 52 (25.0%) changed or added relevant lines in 4 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage decreased (-0.3%) to 45.668%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
pkg/utils/kubeconfig.go	0	2	0.0%
pkg/provisioner/provisioner.go	0	13	0.0%
pkg/provider/aws/create.go	12	36	33.33%

Totals
Change from base Build 21755804296:	-0.3%
Covered Lines:	2077
Relevant Lines:	4548

💛 - Coveralls

Copilot

Pull request overview

This PR aims to eliminate previously ignored errors by either logging failures (for status updates) or documenting why certain cleanup errors are safe to ignore. In addition, it introduces new CLI commands and expands the CLI entrypoint wiring/help text.

Changes:

Add warning logs when AWS provider status-condition updates fail (pkg/provider/aws/create.go).
Replace //nolint:errcheck///nolint:gosec on various Close() calls with explanatory comments (pkg/provisioner/*, pkg/utils/*).
Add new CLI commands (validate, provision) and update CLI command registration/help text (cmd/cli/*).

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 20 comments.

Show a summary per file

File	Description
pkg/utils/kubeconfig.go	Replaces nolint’d Close() calls with “Ignore” comments during cleanup.
pkg/utils/ip.go	Replaces nolint’d `resp.Body.Close()` with an “Ignore” comment.
pkg/provisioner/provisioner.go	Documents previously nolint’d Close() calls across provisioning/cleanup flows.
pkg/provider/aws/create.go	Logs warnings when condition/status updates fail instead of silently ignoring errors.
cmd/cli/validate/validate.go	Adds a new `holodeck validate` command to preflight environment configs.
cmd/cli/provision/provision.go	Adds a new `holodeck provision` command (instance mode + SSH mode).
cmd/cli/main.go	Registers additional CLI commands and expands CLI help examples.

Copilot · 2026-02-04T16:33:03Z

pkg/provisioner/provisioner.go

@@ -250,7 +250,7 @@ func (p *Provisioner) createKindConfig(env v1alpha1.Environment) error {
 	if err != nil {
 		return fmt.Errorf("failed to create session: %v", err)
 	}
-	defer session.Close() // nolint:errcheck, gosec
+	defer session.Close() // Ignore: cleanup error - resource is being cleaned up anyway


defer session.Close() ignores the returned error and will be flagged by gosec now that the nolint is removed. Handle/log the close error (or keep an explicit linter suppression).

Copilot · 2026-02-04T16:33:04Z

pkg/provisioner/provisioner.go

 	// Close the writing pipe and wait for the session to finish
-	remoteFile.Close() // nolint:errcheck, gosec
+	remoteFile.Close() // Ignore: cleanup error - resource is being cleaned up anyway
 	if err := session.Wait(); err != nil {


remoteFile.Close() returns an error; gosec will still flag this as unchecked (G104) without the nolint. Handle/log the error (and consider defering the close right after StdinPipe() succeeds to simplify the control flow).

Copilot · 2026-02-04T16:33:04Z

pkg/provisioner/provisioner.go

 	if err := session.Run("sudo mkdir -p /etc/kubernetes"); err != nil {
-		session.Close() // nolint:errcheck, gosec
+		session.Close() // Ignore: cleanup error - resource is being cleaned up anyway
 		return fmt.Errorf("failed to create directory on remote host: %v", err)
 	}
-	session.Close() // nolint:errcheck, gosec
+	session.Close() // Ignore: cleanup error - resource is being cleaned up anyway


These session.Close() calls still ignore returned errors; gosec will flag them now that the nolint is removed (G104). Capture/log the Close error (or restructure with a single defer so you don't need multiple closes in the error paths).

Copilot · 2026-02-04T16:33:04Z

cmd/cli/main.go

+	"github.com/NVIDIA/holodeck/cmd/cli/describe"
 	"github.com/NVIDIA/holodeck/cmd/cli/dryrun"
+	"github.com/NVIDIA/holodeck/cmd/cli/get"
 	"github.com/NVIDIA/holodeck/cmd/cli/list"
 	oscmd "github.com/NVIDIA/holodeck/cmd/cli/os"


The CLI imports/registers commands (describe/get/scp/ssh/update) that do not exist under cmd/cli in this PR, which will break go build for the CLI. Either add these packages (and their NewCommand exports) or remove the imports/command registrations and related examples from the help text.

Copilot · 2026-02-04T16:33:05Z

pkg/utils/ip.go

 	if err != nil {
 		return "", fmt.Errorf("error fetching IP from %s: %v", url, err)
 	}
-	defer resp.Body.Close() // nolint:errcheck, gosec, staticcheck
+	defer resp.Body.Close() // Ignore: cleanup error - response body is being cleaned up anyway



defer resp.Body.Close() ignores the returned error; gosec (G307) will flag this without the nolint. Wrap it in a deferred func that checks/logs the close error (or keep a targeted //nolint:gosec with justification).

Copilot · 2026-02-04T16:33:08Z

pkg/provisioner/provisioner.go

 		session, err := p.Client.NewSession()
 		if err == nil {
-			session.Close() // nolint:errcheck, gosec
+			session.Close() // Ignore: cleanup error - resource is being cleaned up anyway
 			// Connection is alive, close it
 			if err := p.Client.Close(); err != nil {
 				return fmt.Errorf("failed to close ssh client: %w", err)


Same issue here: session.Close() returns an error and gosec will flag the unchecked error (G104). Handle/log the Close error rather than only documenting it in a comment.

Copilot · 2026-02-04T16:33:08Z

pkg/provisioner/provisioner.go

 	if err := session.Run(fmt.Sprintf("sudo mv %s %s", tempRemotePath, remoteFilePath)); err != nil {
-		session.Close() // nolint:errcheck, gosec
+		session.Close() // Ignore: cleanup error - resource is being cleaned up anyway
 		return fmt.Errorf("failed to move kubeadm config to final destination: %v", err)
 	}
-	session.Close() // nolint:errcheck, gosec
+	session.Close() // Ignore: cleanup error - resource is being cleaned up anyway


Same here: session.Close() errors are still unchecked and will be flagged by gosec. Handle/log the Close error rather than relying on a comment.

Copilot · 2026-02-04T16:33:08Z

pkg/provisioner/provisioner.go

 	if err != nil {
 		return fmt.Errorf("failed to start SFTP session: %v", err)
 	}
-	defer client.Close() // nolint:errcheck, gosec
+	defer client.Close() // Ignore: cleanup error - resource is being cleaned up anyway



defer client.Close() ignores the returned error; gosec will flag this as an unchecked error (G104/G307). Wrap in a deferred func and log (or otherwise handle) the close error.

Copilot · 2026-02-04T16:33:08Z

pkg/utils/kubeconfig.go

 		return fmt.Errorf("error creating local file: %v", err)
 	}
-	defer localFile.Close() // nolint:errcheck, gosec
+	defer localFile.Close() // Ignore: cleanup error - resource is being cleaned up anyway


defer localFile.Close() ignores the returned error and will be flagged by gosec (G307). Prefer a deferred func that captures/logs the Close error, especially since this is writing the kubeconfig to disk.

Suggested change

defer localFile.Close() // Ignore: cleanup error - resource is being cleaned up anyway

defer func() {

if cerr := localFile.Close(); cerr != nil {

log.Info(fmt.Sprintf("error closing local kubeconfig file %s: %v", dest, cerr))

}

}()

Copilot · 2026-02-04T16:33:08Z

pkg/provisioner/provisioner.go


 	go func() {
-		defer writer.Close() // nolint:errcheck, gosec
+		defer writer.Close() // Ignore: cleanup error - resource is being cleaned up anyway


defer writer.Close() ignores the returned error; gosec will still flag this without the nolint (G104/G307 depending on rule). Wrap the Close in a deferred func that checks/logs the error.

Suggested change

defer writer.Close() // Ignore: cleanup error - resource is being cleaned up anyway

defer func() {

if err := writer.Close(); err != nil {

logger.Error("failed to close pipe writer", "error", err)

}

}()

- Replace ignored status update errors in create.go with proper logging - 12 status update calls now log warnings instead of silently ignoring errors - updateProgressingCondition and updateDegradedCondition errors are now visible - Document Close() errors in provisioner.go, kubeconfig.go, and ip.go - Added explanatory comments for 14 Close() operations - These cleanup errors are acceptable as resources are being cleaned up anyway All changes compile successfully. Status update failures are now visible in logs, improving observability of state tracking issues. Signed-off-by: Carlos Eduardo Arango Gutierrez <eduardoa@nvidia.com>

…IA#610) - Replace ignored status update errors in create.go with proper logging - 12 status update calls now log warnings instead of silently ignoring errors - updateProgressingCondition and updateDegradedCondition errors are now visible - Document Close() errors in provisioner.go, kubeconfig.go, and ip.go - Added explanatory comments for 14 Close() operations - These cleanup errors are acceptable as resources are being cleaned up anyway All changes compile successfully. Status update failures are now visible in logs, improving observability of state tracking issues. Signed-off-by: Carlos Eduardo Arango Gutierrez <eduardoa@nvidia.com>

Copilot AI review requested due to automatic review settings February 4, 2026 16:23

Copilot started reviewing on behalf of ArangoGutierrez February 4, 2026 16:23 View session

Copilot AI reviewed Feb 4, 2026

View reviewed changes

ArangoGutierrez force-pushed the fix/address-ignored-errors branch 2 times, most recently from 1b1ece5 to f9354a5 Compare February 6, 2026 15:01

ArangoGutierrez enabled auto-merge (squash) February 6, 2026 15:03

ArangoGutierrez force-pushed the fix/address-ignored-errors branch from f9354a5 to ac00335 Compare February 6, 2026 15:26

ArangoGutierrez merged commit e8f5f7d into NVIDIA:main Feb 6, 2026
8 of 9 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(pkg): address ignored errors with logging and documentation#610

fix(pkg): address ignored errors with logging and documentation#610
ArangoGutierrez merged 1 commit intoNVIDIA:mainfrom
ArangoGutierrez:fix/address-ignored-errors

ArangoGutierrez commented Feb 4, 2026

Uh oh!

coveralls commented Feb 4, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Feb 4, 2026

Uh oh!

Copilot AI Feb 4, 2026

Uh oh!

Copilot AI Feb 4, 2026

Uh oh!

Copilot AI Feb 4, 2026

Uh oh!

Copilot AI Feb 4, 2026

Uh oh!

Copilot AI Feb 4, 2026

Uh oh!

Copilot AI Feb 4, 2026

Uh oh!

Copilot AI Feb 4, 2026

Uh oh!

Copilot AI Feb 4, 2026

Uh oh!

Copilot AI Feb 4, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

-	defer localFile.Close() // Ignore: cleanup error - resource is being cleaned up anyway
+	defer func() {
+		if cerr := localFile.Close(); cerr != nil {
+			log.Info(fmt.Sprintf("error closing local kubeconfig file %s: %v", dest, cerr))
+		}
+	}()

Conversation

ArangoGutierrez commented Feb 4, 2026

Summary

Changes

pkg/provider/aws/create.go (12 errors fixed)

pkg/provisioner/provisioner.go (13 errors documented)

pkg/utils/kubeconfig.go & ip.go (3 errors documented)

Impact

Test plan

Uh oh!

coveralls commented Feb 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Test Coverage Report for Build 21755908155

Warning: This coverage report may be inaccurate.

Details

💛 - Coveralls

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Feb 4, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 4, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 4, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 4, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 4, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 4, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 4, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 4, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 4, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 4, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

coveralls commented Feb 4, 2026 •

edited

Loading