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

Add GetRevision() #92

Closed
wants to merge 1 commit into from
Closed

Add GetRevision() #92

wants to merge 1 commit into from

Conversation

kozlovic
Copy link
Member

GetRevision returns a specific revision value for the key, or "KeyNotFound" if that message for this revision can't be found. See nats-io/nats.go#903

@kozlovic
Copy link
Member Author

@ripienaar Not sure if this is needed in the ADR. Regardless, since we have added that to the Go client, I will create an issue so that other libs can add it.

@kozlovic kozlovic mentioned this pull request Feb 10, 2022
12 tasks
@ripienaar
Copy link
Contributor

I think to make it for the ADR

@ripienaar Not sure if this is needed in the ADR. Regardless, since we have added that to the Go client, I will create an issue so that other libs can add it.

I think to be suitable for the ADR we should consider rounding out the history feature set a bit - for example a API call to take revision x and promote it to the current value for a key.

I kept it out initially as I wanted to see how the history / audit feature shakes out. Could be fine to mark this as optional also and leave as is?

@kozlovic
Copy link
Member Author

Could be fine to mark this as optional also and leave as is?

Just searched for "optional" in that whole PR and don't find it, so that would be the only one with that comment, so not sure this is the right approach then. Again, I have created the issue so that libraries can provide this feature, but maybe it is indeed optional and does not have to be in the ADR. In other words, happy to close this PR without a merge..

@ripienaar
Copy link
Contributor

There's a few phrases like language implementations can add what makes sense in addition., essentially ADR is the core feature set but things like GetString(), GetBytes() etc exist in some - but not for everyone to adopt.

We can do that way or as you did, same same. I am not sure where we stand with stabalizing the API but imo the goal was we're not supposed to add new required things at this point - could list it in one of the future roadmap items above as a future delivery.

GetRevision returns a specific revision value for the key, or "KeyNotFound" if that message for this revision can't be found. See nats-io/nats.go#903

Signed-off-by: Ivan Kozlovic <ivan@synadia.com>
@kozlovic
Copy link
Member Author

@ripienaar I removed from the kv-readonly section and added as a note where we say that we can add GetUint64() or the like. Feels a bit lost there and makes me really wonder if it is worth updating the ADR.
Again, since this is considered optional, then I am not certain we need to update this, simply have the issue that I have created so that devs can implement it if they want their lib to have the API..

@ripienaar
Copy link
Contributor

Yeah I agree, if its optional I wouldnt bother with the ADR. Later when we choose to add a set of audit/history related management things it makes sense in the ADR, in isolation its a bit low value

@kozlovic
Copy link
Member Author

Closing then, since there is no value adding it there. Thanks!

@kozlovic kozlovic closed this Feb 11, 2022
@scottf scottf deleted the kozlovic-patch-1 branch September 20, 2022 15:27
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

2 participants