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

Preserve ModifyIndex for unchanged entry in KVS TXN #7832

Merged
merged 3 commits into from
May 14, 2020

Conversation

a-zagaevskiy
Copy link
Contributor

These changes are made to preserve ModifyIndex for entries that are left untouched in the KVStore during a transaction.

@freddygv
Copy link
Contributor

Hi @a-zagaevskiy, can you please add a test to /agent/consul/state/txn_test.go?

It would be based on this following test, but would run multiple KVSet operations instead of running every possible operation once:

func TestStateStore_Txn_KVS(t *testing.T) {

@a-zagaevskiy
Copy link
Contributor Author

Hi @freddygv ! Is it okay?

To be honest, I'm not sure that I chose an appropriate name for the test. And there are some doubts about how I've set up that commits. But I would be glad for your help.

Copy link
Contributor

@freddygv freddygv left a comment

Choose a reason for hiding this comment

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

This looks good, I made one other minor comment.

agent/consul/state/kvs.go Show resolved Hide resolved
Copy link
Contributor

@freddygv freddygv left a comment

Choose a reason for hiding this comment

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

Thanks for the issue and fix in a PR @a-zagaevskiy. This is good to merge now!

@freddygv freddygv changed the title Preserve ModifyIndex for unchanged entry Preserve ModifyIndex for unchanged entry in KVS TXN May 14, 2020
@freddygv freddygv merged commit a75e3d9 into hashicorp:master May 14, 2020
@a-zagaevskiy
Copy link
Contributor Author

Glad I could help a little!

And what about the corresponding issue? Should I close it manually right now? It looks like I didn't link it properly with this PR.

@freddygv
Copy link
Contributor

Ah yep. In order to auto-close issues the PR description needs contain:
Fixes #7826

I just closed the issue though. Thanks for the reminder.

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.

3 participants