From cecc46c5fe821794f3cc974abe3bc36dec113652 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=D7=A0=CF=85=CE=B1=CE=B7=20=D7=A0=CF=85=CE=B1=CE=B7=D1=95?= =?UTF-8?q?=CF=83=CE=B7?= Date: Thu, 1 Oct 2026 21:33:59 -0700 Subject: [PATCH] fix(file): errors read as the convention says Applies osapi-io/specs#245 to the file provider, which carried the worst of the four shapes. Every error here began "failed to", which is the one Go's own guidance argues against: these are almost always wrapped, so the reader saw "deploy schedule entry: failed to execute template: ..." where the words carried nothing the context had not. They now read "file deploy: ...", "file template: ...", "file status: ..." and "file undeploy: ...", named for the operation rather than for the fact that something went wrong. Two things the tests caught. The ownership error spans two lines, so a regex anchored on fmt.Errorf missed it. And the template failures are wrapped by deploy rather than raised by template, so the assertion naming the originating operation was wrong until it was read off the actual error. Five providers still carry the older shapes. This is the first. Refs #565 Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01FuKUsHFG1EqZXamffh9M2c --- internal/provider/file/deploy.go | 24 ++++++++--------- internal/provider/file/deploy_public_test.go | 26 +++++++++---------- internal/provider/file/status.go | 2 +- internal/provider/file/status_public_test.go | 2 +- internal/provider/file/template.go | 4 +-- .../provider/file/template_public_test.go | 6 ++--- internal/provider/file/undeploy.go | 2 +- .../provider/file/undeploy_public_test.go | 2 +- 8 files changed, 34 insertions(+), 34 deletions(-) diff --git a/internal/provider/file/deploy.go b/internal/provider/file/deploy.go index f46e28197..2807e7914 100644 --- a/internal/provider/file/deploy.go +++ b/internal/provider/file/deploy.go @@ -48,7 +48,7 @@ func (p *Service) Deploy( ) (*DeployResult, error) { content, err := p.objStore.GetBytes(ctx, req.ObjectName) if err != nil { - return nil, fmt.Errorf("failed to get object %q: %w", req.ObjectName, err) + return nil, fmt.Errorf("file deploy: get object %q: %w", req.ObjectName, err) } // When ContentType is not explicitly set, resolve it from the object's @@ -69,7 +69,7 @@ func (p *Service) Deploy( if contentType == "template" { content, err = p.renderTemplate(content, req.Vars) if err != nil { - return nil, fmt.Errorf("failed to render template: %w", err) + return nil, fmt.Errorf("file deploy: render template: %w", err) } } @@ -117,11 +117,11 @@ func (p *Service) Deploy( dir := p.fs.Dir(req.Path) if err := p.fs.MkdirAll(dir, 0o755); err != nil { - return nil, fmt.Errorf("failed to create directory %q: %w", dir, err) + return nil, fmt.Errorf("file deploy: create directory %q: %w", dir, err) } if err := fsutil.WriteFileAtomic(p.fs, req.Path, content, mode); err != nil { - return nil, fmt.Errorf("failed to write file %q: %w", req.Path, err) + return nil, fmt.Errorf("file deploy: write file %q: %w", req.Path, err) } if _, err := p.enforceOwnership(ctx, req); err != nil { @@ -170,12 +170,12 @@ func (p *Service) applyPermissions( info, err := p.fs.Stat(req.Path) if err != nil { - return false, fmt.Errorf("failed to stat file %q: %w", req.Path, err) + return false, fmt.Errorf("file deploy: stat file %q: %w", req.Path, err) } if info.Mode().Perm() != want.Perm() { if err := p.fs.Chmod(req.Path, want); err != nil { - return false, fmt.Errorf("failed to set mode on %q: %w", req.Path, err) + return false, fmt.Errorf("file deploy: set mode on %q: %w", req.Path, err) } changed = true @@ -202,17 +202,17 @@ func (p *Service) enforceOwnership( wantUID, err := resolveID(req.Owner, lookupUID) if err != nil { - return false, fmt.Errorf("failed to resolve owner for %q: %w", req.Path, err) + return false, fmt.Errorf("file deploy: resolve owner for %q: %w", req.Path, err) } wantGID, err := resolveID(req.Group, lookupGID) if err != nil { - return false, fmt.Errorf("failed to resolve group for %q: %w", req.Path, err) + return false, fmt.Errorf("file deploy: resolve group for %q: %w", req.Path, err) } matches, err := ownershipMatches(req.Path, wantUID, wantGID) if err != nil { - return false, fmt.Errorf("failed to read ownership of %q: %w", req.Path, err) + return false, fmt.Errorf("file deploy: read ownership of %q: %w", req.Path, err) } if matches { @@ -232,7 +232,7 @@ func (p *Service) enforceOwnership( []string{spec, req.Path}, ); err != nil { return false, fmt.Errorf( - "failed to set ownership %q on %q: %w", + "file deploy: set ownership %q on %q: %w", spec, req.Path, err, @@ -264,11 +264,11 @@ func (p *Service) putState( stateBytes, err := marshalJSON(state) if err != nil { - return fmt.Errorf("failed to marshal file state: %w", err) + return fmt.Errorf("file deploy: marshal file state: %w", err) } if _, err := p.stateKV.Put(ctx, stateKey, stateBytes); err != nil { - return fmt.Errorf("failed to update file state: %w", err) + return fmt.Errorf("file deploy: update file state: %w", err) } return nil diff --git a/internal/provider/file/deploy_public_test.go b/internal/provider/file/deploy_public_test.go index bdbd8d517..0c5e88d1d 100644 --- a/internal/provider/file/deploy_public_test.go +++ b/internal/provider/file/deploy_public_test.go @@ -102,7 +102,7 @@ func (suite *DeployPublicTestSuite) TestDeploy() { ContentType: "raw", }, wantErr: true, - wantErrMsg: "failed to marshal file state", + wantErrMsg: "file deploy: marshal file state", }, { name: "when deploy succeeds (new file)", @@ -247,7 +247,7 @@ func (suite *DeployPublicTestSuite) TestDeploy() { ContentType: "raw", }, wantErr: true, - wantErrMsg: "failed to get object", + wantErrMsg: "file deploy: get object", }, { name: "when content type is template", @@ -391,7 +391,7 @@ func (suite *DeployPublicTestSuite) TestDeploy() { ContentType: "raw", }, wantErr: true, - wantErrMsg: "failed to write file", + wantErrMsg: "file deploy: write file", }, { name: "when mkdir fails", @@ -425,7 +425,7 @@ func (suite *DeployPublicTestSuite) TestDeploy() { ContentType: "raw", }, wantErr: true, - wantErrMsg: "failed to create directory", + wantErrMsg: "file deploy: create directory", }, { name: "when state KV put fails", @@ -449,7 +449,7 @@ func (suite *DeployPublicTestSuite) TestDeploy() { ContentType: "raw", }, wantErr: true, - wantErrMsg: "failed to update file state", + wantErrMsg: "file deploy: update file state", }, { name: "when the mode cannot be parsed on a file already correct", @@ -681,7 +681,7 @@ func (suite *DeployPublicTestSuite) TestDeploy() { ContentType: "raw", }, wantErr: true, - wantErrMsg: "failed to resolve owner", + wantErrMsg: "file deploy: resolve owner", }, { name: "when the host does not know the requested group", @@ -712,7 +712,7 @@ func (suite *DeployPublicTestSuite) TestDeploy() { ContentType: "raw", }, wantErr: true, - wantErrMsg: "failed to resolve group", + wantErrMsg: "file deploy: resolve group", }, { name: "when the file ownership cannot be read at all", @@ -743,7 +743,7 @@ func (suite *DeployPublicTestSuite) TestDeploy() { ContentType: "raw", }, wantErr: true, - wantErrMsg: "failed to read ownership", + wantErrMsg: "file deploy: read ownership", }, { name: "when the platform cannot report ownership it is applied anyway", @@ -888,7 +888,7 @@ func (suite *DeployPublicTestSuite) TestDeploy() { ContentType: "raw", }, wantErr: true, - wantErrMsg: "failed to set ownership", + wantErrMsg: "file deploy: set ownership", }, { name: "when the file cannot be stat'd while enforcing the mode", @@ -929,7 +929,7 @@ func (suite *DeployPublicTestSuite) TestDeploy() { ContentType: "raw", }, wantErr: true, - wantErrMsg: "failed to stat file", + wantErrMsg: "file deploy: stat file", }, { name: "when the mode cannot be applied", @@ -969,7 +969,7 @@ func (suite *DeployPublicTestSuite) TestDeploy() { ContentType: "raw", }, wantErr: true, - wantErrMsg: "failed to set mode", + wantErrMsg: "file deploy: set mode", }, { name: "when chown fails on a file whose content is already correct", @@ -1005,7 +1005,7 @@ func (suite *DeployPublicTestSuite) TestDeploy() { ContentType: "raw", }, wantErr: true, - wantErrMsg: "failed to set ownership", + wantErrMsg: "file deploy: set ownership", }, { name: "when recording a permission change fails", @@ -1033,7 +1033,7 @@ func (suite *DeployPublicTestSuite) TestDeploy() { ContentType: "raw", }, wantErr: true, - wantErrMsg: "failed to update file state", + wantErrMsg: "file deploy: update file state", }, } diff --git a/internal/provider/file/status.go b/internal/provider/file/status.go index 6959e2352..806e68210 100644 --- a/internal/provider/file/status.go +++ b/internal/provider/file/status.go @@ -47,7 +47,7 @@ func (p *Service) Status( var state job.FileState if err := json.Unmarshal(entry.Value(), &state); err != nil { - return nil, fmt.Errorf("failed to parse file state: %w", err) + return nil, fmt.Errorf("file status: parse file state: %w", err) } data, err := p.fs.ReadFile(req.Path) diff --git a/internal/provider/file/status_public_test.go b/internal/provider/file/status_public_test.go index 3a5b7a72f..6d362c7b0 100644 --- a/internal/provider/file/status_public_test.go +++ b/internal/provider/file/status_public_test.go @@ -183,7 +183,7 @@ func (suite *StatusPublicTestSuite) TestStatus() { }, validateFunc: func(got *file.StatusResult, err error) { suite.Error(err) - suite.ErrorContains(err, "failed to parse file state") + suite.ErrorContains(err, "file status: parse file state") suite.Nil(got) }, }, diff --git a/internal/provider/file/template.go b/internal/provider/file/template.go index 5be71f266..e419a0338 100644 --- a/internal/provider/file/template.go +++ b/internal/provider/file/template.go @@ -44,7 +44,7 @@ func (p *Service) renderTemplate( ) ([]byte, error) { tmpl, err := template.New("file").Option("missingkey=error").Parse(string(rawTemplate)) if err != nil { - return nil, fmt.Errorf("failed to parse template: %w", err) + return nil, fmt.Errorf("file template: parse template: %w", err) } ctx := TemplateContext{ @@ -55,7 +55,7 @@ func (p *Service) renderTemplate( var buf bytes.Buffer if err := tmpl.Execute(&buf, ctx); err != nil { - return nil, fmt.Errorf("failed to execute template: %w", err) + return nil, fmt.Errorf("file template: execute template: %w", err) } return buf.Bytes(), nil diff --git a/internal/provider/file/template_public_test.go b/internal/provider/file/template_public_test.go index d38d65254..a10269f74 100644 --- a/internal/provider/file/template_public_test.go +++ b/internal/provider/file/template_public_test.go @@ -166,7 +166,7 @@ func (suite *TemplatePublicTestSuite) TestDeployTemplate() { wantErr: true, validateFunc: func(got *file.DeployResult, err error, _ avfs.VFS) { suite.Error(err) - suite.ErrorContains(err, "failed to render template") + suite.ErrorContains(err, "file deploy: render template") suite.Nil(got) }, }, @@ -177,7 +177,7 @@ func (suite *TemplatePublicTestSuite) TestDeployTemplate() { wantErr: true, validateFunc: func(got *file.DeployResult, err error, _ avfs.VFS) { suite.Error(err) - suite.ErrorContains(err, "failed to render template") + suite.ErrorContains(err, "file deploy: render template") suite.Nil(got) }, }, @@ -189,7 +189,7 @@ func (suite *TemplatePublicTestSuite) TestDeployTemplate() { wantErr: true, validateFunc: func(got *file.DeployResult, err error, _ avfs.VFS) { suite.Error(err) - suite.ErrorContains(err, "failed to render template") + suite.ErrorContains(err, "file deploy: render template") suite.Nil(got) }, }, diff --git a/internal/provider/file/undeploy.go b/internal/provider/file/undeploy.go index 6843fe2dd..c19c2f08e 100644 --- a/internal/provider/file/undeploy.go +++ b/internal/provider/file/undeploy.go @@ -51,7 +51,7 @@ func (p *Service) Undeploy( } if err := p.fs.Remove(req.Path); err != nil { - return nil, fmt.Errorf("failed to remove file %q: %w", req.Path, err) + return nil, fmt.Errorf("file undeploy: remove file %q: %w", req.Path, err) } stateKey := BuildStateKey(p.hostname, req.Path) diff --git a/internal/provider/file/undeploy_public_test.go b/internal/provider/file/undeploy_public_test.go index bac924f4b..3d8a9ecdb 100644 --- a/internal/provider/file/undeploy_public_test.go +++ b/internal/provider/file/undeploy_public_test.go @@ -195,7 +195,7 @@ func (suite *UndeployPublicTestSuite) TestUndeploy() { }, req: file.UndeployRequest{Path: "/etc/cron.d/locked"}, wantErr: true, - wantErrMsg: "failed to remove file", + wantErrMsg: "file undeploy: remove file", useFailFs: true, }, {