feat: Add Annotated Tag Push Support #1139
Conversation
# Conflicts: # src/ui/views/OpenPushRequests/components/PushesTable.tsx # src/ui/views/PushDetails/PushDetails.tsx
✅ Deploy Preview for endearing-brigadeiros-63f9d0 canceled.
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1139 +/- ##
==========================================
- Coverage 82.83% 82.68% -0.15%
==========================================
Files 66 67 +1
Lines 2784 2911 +127
Branches 334 376 +42
==========================================
+ Hits 2306 2407 +101
- Misses 431 446 +15
- Partials 47 58 +11 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| } else if (action.tagData?.length) { | ||
| action.user = action.tagData.at(-1)!.tagger; | ||
| } else { | ||
| throw new Error('No commit or tag data parsed from packfile'); |
There was a problem hiding this comment.
It seems that the commitData check here is causing this test to fail:
it.only('should return empty commitData on empty branch push', async () => {
const emptyPackBuffer = createEmptyPackBuffer();
const newCommit = 'b'.repeat(40);
const ref = 'refs/heads/feature/emptybranch';
const packetLine = `${EMPTY_COMMIT_HASH} ${newCommit} ${ref}\0capabilities\n`;
req.body = Buffer.concat([createPacketLineBuffer([packetLine]), emptyPackBuffer]);
const result = await exec(req, action);
expect(result).to.equal(action);
const step = action.steps.find(s => s.stepName === 'parsePackFile');
expect(step).to.exist;
expect(step.error).to.be.false; // <-- Fails here
expect(action.branch).to.equal(ref);
expect(action.setCommit.calledOnceWith(EMPTY_COMMIT_HASH, newCommit)).to.be.true;
expect(action.commitData).to.be.an('array').with.lengthOf(0);
});Ideally, we want to allow empty commitData so that we can interpret it later: It might be an empty branch push, a tag push or something else entirely.
Perhaps we could move this check into the another action? We could rename the checkEmptyBranch action to handleEmptyCommitData and do all the relevant checks there.
There was a problem hiding this comment.
The core flow seems to work correctly! Just a few edge cases and an integration issue with the checkEmptyBranch that might be worth refactoring.
Also, we will be merging #973 soon which will likely cause a few conflicts here... Might want an extra pair of eyes to make sure everything is good to merge.
|
Updated with the latest changes of main and manually tested also with gitlab |
| action = await proc.pre.parseAction(req); | ||
| // 2) Parse the push payload first to detect tags/branches | ||
| if (action.type === RequestType.PUSH) { | ||
| action = await proc.push.parsePush(req, action); |
There was a problem hiding this comment.
I'm not sure if I misunderstood the flow here, but isn't parsePush executing twice when pushing branches? Seems it executes here, and once again as part of the branchPushChain.
There was a problem hiding this comment.
Regardless, it seems the push flow is working as expected - just not sure if this is running twice or if the parsePush prevents the double execution somehow.
|
As we're modifying the proxy logic, another pair of eyes here would be super helpful! 👀 @finos/git-proxy-maintainers |
Description:
This PR introduces end-to-end support for annotated tag pushes
branchPushChainandtagPushChain.getChain()to select the correct chain based onaction.typeand presence ofaction.tag.Fixes #986
Note: Restored from deleted fork
This PR recreates the original PR #1051, which was automatically closed due to accidental fork deletion.
** For discussions and reviews:** See the original PR #1051
All commits are identical to the original with preserved git history.