Skip to content
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.

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

Allow top-level Configure to return an error #197

Merged
merged 1 commit into from
Mar 16, 2024

Conversation

elezar
Copy link
Contributor

@elezar elezar commented Mar 7, 2024

No description provided.

Signed-off-by: Evan Lezar <elezar@nvidia.com>
@elezar elezar requested a review from klihub March 7, 2024 15:59
Copy link
Contributor

@klihub klihub left a comment

Choose a reason for hiding this comment

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

LGTM.

@klihub
Copy link
Contributor

klihub commented Mar 7, 2024

For context: We discussed last week whether cache.Configure() should be changed to not return an error, since #187 got merged and it changed Option to not have the possibility to return errors. But we concluded that #187 might have been not such a good idea, as we might want to add an Option in the future which should be able to fail. At that point we'd need to flip cache.Configure() back to return an error, so we might as well omit that API-braking change now instead of revert it in the future. But the package-level/default cache Configure() should have the same signature as cache.Configure() (after all that's what it boils down to), and since the consensus now is that we don't remove error from the latter, the former should updated to reflect this, too.

Copy link
Contributor

@marquiz marquiz left a comment

Choose a reason for hiding this comment

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

LGTM. Makes sense to me

@klihub klihub merged commit 780f60f into cncf-tags:main Mar 16, 2024
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants