Skip to content

Return a wrapped fs.ErrNotExist if no spec directories are defined #309

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
elezar wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Return a wrapped fs.ErrNotExist if no spec directories are defined #309

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion pkg/cdi/cache.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -321,7 +321,7 @@ func (c *Cache) RemoveSpec(name string) error {

specDir, _ = c.highestPrioritySpecDir()
if specDir == "" {
Copy link
Copy Markdown
Contributor

@klihub klihub Feb 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@elezar Can this still happen with #310 merged ?

Copy link
Copy Markdown
Contributor Author

@elezar elezar Feb 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It shouldn't be possible, no.

Copy link
Copy Markdown
Contributor Author

@elezar elezar Feb 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does that mean we want to update the highestPrioritySpecDir implementation to not return an error?

Copy link
Copy Markdown
Contributor

@klihub klihub Feb 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it does not return an error. It returns a directory and its priority.

return errors.New("no Spec directories to remove from")
return fmt.Errorf("no Spec directories defined: %w", fs.ErrNotExist)
Copy link
Copy Markdown
Contributor

@fujitatomoya fujitatomoya Feb 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the fix looks good to me, but how about adding test case for this?

err := cache.RemoveSpec("something")
assert.True(t, errors.Is(err, fs.ErrNotExist))

Copy link
Copy Markdown
Contributor Author

@elezar elezar Mar 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have added a basic test.

Copy link
Copy Markdown
Contributor Author

@elezar elezar Mar 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@klihub as also pointed out by the test, we do have a slight disconnect here. In the case where the spec dirs are empty, we return ErrNotExist, but in the case where os.Remove fails for the spec we are trying to remove due to ErrNotExist we return nil. Should we always return nil (or ErrNotExist) for consistency?

Copy link
Copy Markdown
Contributor

@klihub klihub Mar 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@elezar Yes, probably would make sense. Returning ErrNotExist as such would provide transparency for RemoveSpec() while returnin nil for ErrNotExist would provide idempotency. From the top of my head I don't know which would be better.

Copy link
Copy Markdown
Contributor Author

@elezar elezar Mar 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think at this stage switching to returning ErrNotExists will end up breaking producers that use this code. I'm not sure if that's something we want to do. Let me add a comment to the code to this effect.

Also, as a follow-up, does this mean that we should rather return nil if there are no spec directories (especially given that this is now a case that we don't expect to ever occur)?

Copy link
Copy Markdown
Contributor

@klihub klihub Mar 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, as a follow-up, does this mean that we should rather return nil if there are no spec directories (especially given that this is now a case that we don't expect to ever occur)?

On the RemoveSpec() code path I think we could return nil if we have no spec directories if we return nil when there is no spec file to remove, as the former implies the latter.

I think we still might get into the situation when there are no spec dirs, because someone might nuke them while we are running.

Copy link
Copy Markdown
Contributor

@klihub klihub Mar 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think at this stage switching to returning ErrNotExists will end up breaking producers that use this code. I'm not sure if that's something we want to do. Let me add a comment to the code to this effect.

Yes, I also think we should try to be extra conservative even when it feels like PITA. We're post 1.0, so we should not be making breaking changes. Even if it's not immediately obvious why and how someone could rely on existing functionality so that it would break if we start returning ErrNotExists, it's better not to risk especially if there is not much to gain by the change.

}

path = filepath.Join(specDir, name)
Expand Down
40 changes: 40 additions & 0 deletions pkg/cdi/cache_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1839,6 +1839,46 @@ devices:
}
}

func TestRemoveCache(t *testing.T) {

tmpDir := t.TempDir()

testCases := []struct {
name string
cache *Cache
specToRemove string
expectedError error
}{
{
name: "returns error if cache dirs is empty",
cache: &Cache{
specDirs: nil,
},
specToRemove: "any",
expectedError: os.ErrNotExist,
},
{
name: "returns no error if spec does not exist",
cache: &Cache{
specDirs: []string{tmpDir},
},
specToRemove: "non-existent",
expectedError: nil,
},
}

for _, tc := range testCases {
t.Run(tc.name, func(t *testing.T) {
err := tc.cache.RemoveSpec(tc.specToRemove)
if tc.expectedError == nil {
require.NoError(t, err)
} else {
require.ErrorIs(t, err, tc.expectedError)
}
})
}
}

// Create and populate automatically cleaned up spec directories.
func createSpecDirs(t *testing.T, etc, run map[string]string) (string, error) {
return mkTestDir(t, map[string]map[string]string{
Expand Down