-
Notifications
You must be signed in to change notification settings - Fork 640
datastore: fix - mark transactions as finalized. #250
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
datastore: fix - mark transactions as finalized. #250
Conversation
|
Oops. Do we have to wrap the method or can we detect that it's a transaction by looking for |
|
Oh, good call. We can finalize from datastore request if you think that's On Wednesday, October 8, 2014, Ryan Seys notifications@github.com wrote:
|
|
I'd prefer all the logic to be there, yes. Much easier to follow it that way. |
e59a61f to
cc519d4
Compare
cc519d4 to
c79a52d
Compare
|
Updated with the new plan. |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
|
Just worried if the transaction isn't finalized when a response is received, that could have unexpected side-effects. Other than that, this looks good to me :) |
|
Can we test it somehow? Having a test would have avoided the regression. |
|
@silvolu I had the same thought. I figured I'd let this PR go, then attack a re-factor of datastore tests and add it at that time, as well as anything else that's missing. After we put the shared logic into request.js, our test files aren't split to match, so functionality testing is a bit criss-crossed atm. |
|
Ok, I'll merge this, then let's wait for the tests to be fixed before releasing 0.8.1 |
…transactions datastore: fix - mark transactions as finalized.
|
The tests aren't failing so curious what "vector" you're going to take to approach fixing them? Asking for a friend 😉 |
|
Sorry, poor choice of word :) |
|
Makes sense. My apologies for assuming they were sufficient as-is :) Let me know if I can help with the transition. |
BREAKING CHANGE: removes projectPath helper, instead use "projects/${project}".
* updated CHANGELOG.md * updated package.json * updated samples/package.json
- [ ] Regenerate this pull request now. PiperOrigin-RevId: 474338479 Source-Link: googleapis/googleapis@d5d35e0 Source-Link: googleapis/googleapis-gen@efcd3f9 Copy-Tag: eyJwIjoiLmdpdGh1Yi8uT3dsQm90LnlhbWwiLCJoIjoiZWZjZDNmOTM5NjJhMTAzZjY4ZjAwM2UyYTFlZWNkZTZmYTIxNmEyNyJ9
* chore(main): release 3.0.3 * 🦉 Updates from OwlBot post-processor See https://github.com/googleapis/repo-automation-bots/blob/main/packages/owl-bot/README.md Co-authored-by: release-please[bot] <55107282+release-please[bot]@users.noreply.github.com> Co-authored-by: Owl Bot <gcf-owl-bot[bot]@users.noreply.github.com>
BREAKING CHANGE: removes projectPath helper, instead use "projects/${project}".
Co-authored-by: release-please[bot] <55107282+release-please[bot]@users.noreply.github.com>
This PR was generated using Autosynth. 🌈 Synth log will be available here: https://source.cloud.google.com/results/invocations/35a31f24-4249-4b7d-9c51-7f63c556390c/targets - [ ] To automatically regenerate this PR, check this box. Source-Link: googleapis/synthtool@0c868d4
Co-authored-by: release-please[bot] <55107282+release-please[bot]@users.noreply.github.com>
The system tests were dependent on @g-c/common, but didn't declare it. This meant that we unwittingly picked up a semver major change causing build to be failing at master.
The recent API refactoring removed the functionality where
saveanddeleteoperations mark the transaction as finalized. This brings it back.