Conversation
|
|
|
/ok-to-test |
|
/paco review |
Paco Review 🔍This PR adds tag push support to the Gitea provider. It introduces logic to detect Review difficulty: 3/5 (Moderate) — Moderate complexity: touches parsing logic, commit resolution, and tests across multiple files with subtle edge cases around zero SHAs and tag vs branch resolution. 2 new inline comment(s) found. Reviewed commit: 2cf04fc |
![pipelines-as-code[bot]](https://avatars.githubusercontent.com/in/112592?s=60&v=4){kind=small}
|
|
||
| if sha == "" { | ||
| switch { | ||
| case runevent.HeadBranch != "": |
There was a problem hiding this comment.
[HIGH] When the tag API call succeeds and updates sha/runevent.SHA to the peeled commit SHA, the code then falls into the if sha == "" block — which it won't enter since sha is now set. That's correct. However, if the tag API call fails (err != nil), the error is silently swallowed. runevent.SHA was already set to the original tag object SHA before this function was called (tagobjectsha999 in the test). In that case, sha retains the original value passed in (runevent.SHA before the function), and GetSingleCommit is called with the tag object SHA rather than the peeled commit SHA. This may silently produce wrong commit info instead of surfacing the API error. At minimum the error should be logged via v.Logger.
| sha = branchinfo.Commit.ID | ||
| case runevent.PullRequestNumber != 0: | ||
| pr, _, err := v.Client().GetPullRequest(runevent.Organization, runevent.Repository, int64(runevent.PullRequestNumber)) | ||
| if err != nil { |
There was a problem hiding this comment.
[MEDIUM] After the tag resolution block sets sha to the peeled commit SHA, the if sha == "" block is skipped entirely. However runevent.HeadBranch still contains refs/tags/v1.0.0. Later, when GetRepoBranch is used as a fallback (for non-tag, non-PR events where SHA is empty), runevent.HeadBranch would be passed as-is. This is fine for the current logic, but note that in the tag case, runevent.HeadBranch is never cleared of the refs/tags/ prefix, which could affect callers that assume HeadBranch is a plain branch name. Consider stripping the refs/tags/ prefix from HeadBranch when a tag is detected, similar to how PR resolution updates HeadBranch.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2997 +/- ##
==========================================
- Coverage 91.57% 91.56% -0.01%
==========================================
Files 164 164
Lines 12532 12546 +14
==========================================
+ Hits 11476 11488 +12
- Misses 1055 1057 +2
Partials 1 1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
2cf04fc to
9a4a40c
Compare
| branchinfo, _, err := v.Client().GetRepoBranch(runevent.Organization, runevent.Repository, runevent.HeadBranch) | ||
| if err != nil { | ||
| return err | ||
| var tagName string |
There was a problem hiding this comment.
I'd rather do this tag fetching in parse_payload.go, as you can see something similar is done for github in its ParsePayload func
There was a problem hiding this comment.
Thanks you for taking the time for the review @zakisk!
I looked into doing this in gitea/parse_payload.go, but if I get it right, there is a lifecycle constraint with Gitea compared to GitHub:
- GitHub can make API calls in parse_payload.go because it runs as a GitHub App and mints an installation token up front from the controller credentials (github/parse_payload.go:312).
- Gitea/Forgejo uses per-repository tokens stored in the Kubernetes Repository CR. When s.vcx.ParsePayload() is called in sinker.go, the Repository CR hasn't been matched yet (matching depends on event.URL from ParsePayload), so v.giteaClient is nil throughout parse_payload.go.
- GetCommitInfo is called after SetClient initializes v.giteaClient. In fact, GetCommitInfo in gitea.go already handles other ref-to-commit API lookups for the exact same reason (e.g. v.Client().GetRepoBranch for branches and v.Client().GetPullRequest for PRs).
Resolving the annotated tag in GetCommitInfo keeps parse_payload.go purely as a payload parser and reuses the authenticated client once initialized. To me, it looks like github is the exception? Let me know if that makes sense, or I missed the point.
There was a problem hiding this comment.
we can continue with what you've implemented and it would be a follow-up task to not make this complext.
There was a problem hiding this comment.
That would be great @zakisk , I'd love to get this upstream.
When a tag is pushed without commits, Forgejo/Gitea provides head_commit=nil, before=0000000000000000000000000000000000000000, and after=<commit/tag SHA>. Using 'before' resulted in querying zero SHA 0000000000000000000000000000000000000000, failing with 'could not find commit info'. In addition: - Reject and skip push events for deleted refs (where 'after' is zero SHA). - Resolve annotated tag objects to their peeled commit SHA via the Forgejo tag API. - Strip refs/tags/ prefix from HeadBranch for clean branch naming. - Return error if tag lookup fails or cannot resolve commit SHA. Fixes tektoncd#2983 Co-authored-by: Gemini <noreply@google.com> Signed-off-by: Søren Dalby Larsen <sdl@midas-energy.com>
9a4a40c to
fa62c5e
Compare
|
Good catch, thanks! If GetTag fails or returns an empty commit SHA for an annotated tag, falling through leaves the SHA pointing to a tag object which GetSingleCommit cannot resolve anyway. |
This branch has not been deployed
4 participants
📝 Description of the Change
When a tag is pushed without commits, Forgejo/Gitea provides head_commit=nil, before=0000000000000000000000000000000000000000, and after=<commit/tag SHA>. Using 'before' resulted in querying commit 0000000000000000000000000000000000000000, failing with 'could not find commit info'.
In addition:
🔗 Linked GitHub Issue
Fixes #2983
🧪 Testing Strategy
🤖 AI Assistance
✅ Submitter Checklist
fix:,feat:) matches the "Type of Change" I selected above.make testandmake lintlocally to check for and fix anyissues. For an efficient workflow, I have considered installing
pre-commit and running
pre-commit installtoautomate these checks.