Skip to content

Improve cache not found error messages#55505

Merged
rosstimothy merged 1 commit intomasterfrom
tross/cache_not_found_error
Jun 10, 2025
Merged

Improve cache not found error messages#55505
rosstimothy merged 1 commit intomasterfrom
tross/cache_not_found_error

Conversation

@rosstimothy
Copy link
Copy Markdown
Contributor

Existing error message

no value for key "fake-tim" in index name

New error message

types.User "fake-tim" does not exist

### Existing error message

```
no value for key "fake-tim" in index name
```

### New error message

```
types.User "tim2" does not exist
```
@rosstimothy rosstimothy added backport no-changelog Indicates that a PR does not require a changelog entry labels Jun 6, 2025
@rosstimothy rosstimothy marked this pull request as ready for review June 7, 2025 12:02
@github-actions github-actions Bot requested review from espadolini and juliaogris June 7, 2025 12:03
Comment thread lib/cache/store.go
t, ok := s.cache.Get(index, key)
if !ok {
return t, trace.NotFound("no value for key %q in index %v", key, index)
return t, trace.NotFound("%v %q does not exist", reflect.TypeFor[T](), key)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is this error user facing? Can we get to a resource kind string somehow?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It could very well be returned to the user. I can likely restrict T to an interface that implements GetKind.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

That's going to be annoying for 153-style resources - is that better or worse than just requiring a user-friendly name for error messages in store?

...also, we're hitting this error message because failed to find the T, and almost all of our resources don't have a "static" GetKind method, they read the kind from the value. 😬

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It's probably less viable than adding a user friendly identifier, though, it will require more boilerplate to stuff in the appropriate string to each resource stores.

@rosstimothy rosstimothy added this pull request to the merge queue Jun 10, 2025
Merged via the queue into master with commit e6331a7 Jun 10, 2025
49 checks passed
@rosstimothy rosstimothy deleted the tross/cache_not_found_error branch June 10, 2025 15:31
@backport-bot-workflows
Copy link
Copy Markdown
Contributor

@rosstimothy See the table below for backport results.

Branch Result
branch/v18 Create PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backport/branch/v18 no-changelog Indicates that a PR does not require a changelog entry size/sm

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants