feat(python): add tag parameter to Branch.transact() API #9334
Conversation
skshetry
force-pushed
the
feat/python-transaction-tag-parameter
branch
3 times, most recently
from
3d07bcf to
3a5cf8e
Compare
Add optional tag parameter to automatically create tags after successful
transaction completion, eliminating manual commit SHA retrieval.
Usage:
```py
with branch.transact(commit_message='update', tag='v1.0.0') as tx:
tx.object('file.txt').upload('data')
# Tag 'v1.0.0' created on merge commit
```
- Tag created only on transaction success (fail-fast validation)
- Empty tag names treated as None
- Tag accessible via tx.tag attribute
Closes: treeverse#8200
skshetry
force-pushed
the
feat/python-transaction-tag-parameter
branch
from
3a5cf8e to
80c80f9
Compare
| initial_commit = test_branch.commit("initial commit") | ||
| repo.tag("v1.0.0").create(initial_commit) | ||
|
|
||
| with expect_exception_context(ConflictException, "tag already exists"): |
There was a problem hiding this comment.
I wonder why we use this custom function instead of using pytest.raises(..., match=...).
This is equivalent to:
| with expect_exception_context(ConflictException, "tag already exists"): | |
| with pytest.raises(ConflictException, match="tag already exists"): |
| with test_branch.transact(commit_message="my transaction with existing tag", tag="v1.0.0") as tx: | ||
| upload_data(tx, path_and_data) | ||
|
|
||
| # Verify transaction branch was deleted | ||
| with expect_exception_context(NotFoundException): | ||
| repo.branch(tx.id).get_commit() |
There was a problem hiding this comment.
Test should not assume transaction successful.
Need to verify data
There was a problem hiding this comment.
This was supposed to be just a sanity check, as some other tests likely verify the data.
There was a problem hiding this comment.
There are no old tests that verify the data upon failure to tag the commit
There was a problem hiding this comment.
Failure to create a tag does not fail the transaction as it happens after the fact.
There was a problem hiding this comment.
This is you speaking as a developer who read the code.
The test is not supposed to assume that.
If we assumed our code does exactly what we want we wouldn't be writing tests.
There was a problem hiding this comment.
Would you be okay with testing that the branch moved to a new commit (i.e is not the initial_commit)? Would that be enough to verify the transaction was successful?
Or, do you think we should verify the data?
There was a problem hiding this comment.
I think checking the commit should be sufficient
Sorry, it looks like GitHub hides previous pushes nowadays (seems to have happened somewhere around mid 2022), and only shows you latest pushes. 🤦🏽 |
|
Hey @N-o-Z, sorry about leaving this hanging, without any updates. |
3 participants
Change Description
Background
This PR addresses GitHub issue #8200 where users requested the ability to create tags automatically during transactions, eliminating the need to manually retrieve commit SHA and create tags separately.
Bug Fix
N/A - This is a new feature.
New Feature
Added optional
tagparameter toBranch.transact()method for automatic tag creation after successful transaction completion.Usage:
Key behaviors:
tx.tagattribute during transactionTesting Details
Breaking Change?
No. This is a backward-compatible addition - existing transaction code works unchanged.
Additional info
Files modified:
lakefs/branch.py: Added tag parameter and creation logictests/integration/test_branch.py: Added tag functionality testContact Details
Available via GitHub for questions.