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

kv: fix typo on error type #1045

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

kv: fix typo on error type #1045

wants to merge 1 commit into from

Conversation

wallyqs
Copy link
Member

@wallyqs wallyqs commented Aug 11, 2022

Signed-off-by: Waldemar Quevedo wally@nats.io

Signed-off-by: Waldemar Quevedo <wally@nats.io>
@wallyqs wallyqs requested a review from kozlovic August 11, 2022 16:24
Copy link
Member

@kozlovic kozlovic left a comment

Choose a reason for hiding this comment

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

This was released (since v1.13.0) so is that not dangerous to change that exported variable? We can change, but that may break some apps/tests, no?

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.05%) to 85.551% when pulling 920ea56 on kv-err-typo into fb5ca2c on main.

@wallyqs
Copy link
Member Author

wallyqs commented Aug 11, 2022

@kozlovic the server is not using it so looks fine... but agree, no hurry to merge, I thought I should open just in case there is a window to fix still

Copy link
Member

@kozlovic kozlovic left a comment

Choose a reason for hiding this comment

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

LGTM. I guess for the next release v1.17.0, we could put that in the "[CHANGED]" section of the release notes.

@wallyqs
Copy link
Member Author

wallyqs commented Sep 16, 2022

I think we can postpone merging this one for v1.17.0 and instead include it as part eventual rework how the KeyValue errors are handled so that they are APIError aware.

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