savitaashture
commented
1 month ago cc @gabemontero
Closed savitaashture closed 4 weeks ago
savitaashture
commented
1 month ago cc @gabemontero
codecov[bot]
commented
1 month ago Attention: Patch coverage is 94.11765% with 1 line in your changes missing coverage. Please review.
Project coverage is 64.58%. Comparing base (
9dcbe19) to head (eaa4a2c). Report is 3 commits behind head on main.
| Files | Patch % | Lines |
|---|---|---|
| pkg/customparams/standard.go | 0.00% | 1 Missing :warning: |
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
chmouel
commented
1 month ago lgtm
what was decided @savitaashture about the lack of error processing at
because when you create a Merge Request (MR) from a fork in an upstream repository, the token needs to have write access to the fork repository to create a status. Typically, the token set on the Repository CR doesn't have such broad access, so we can't create a status commit on it.
I explained this poorly in a comment when I first implemented it a few years ago here:
I asked our GitLab developer liaison about it but haven't received a response. I'm fairly certain there was a bug report in the GitLab instance addressing this issue. maybe something has changed since then 🤷🏻
gabemontero
commented
1 month ago lgtm what was decided @savitaashture about the lack of error processing at https://github.com/openshift-pipelines/pipelines-as-code/blob/0e97e1d1afead542c99ce8004a9f800b0998efe8/pkg/provider/gitlab/gitlab.go#L213
because when you create a Merge Request (MR) from a fork in an upstream repository, the token needs to have write access to the fork repository to create a status. Typically, the token set on the Repository CR doesn't have such broad access, so we can't create a status commit on it.
I explained this poorly in a comment when I first implemented it a few years ago here:
I asked our GitLab developer liaison about it but haven't received a response. I'm fairly certain there was a bug report in the GitLab instance addressing this issue. maybe something has changed since then 🤷🏻
thanks for the detailed explanation @chmouel .... helps immensely
@savitaashture - in this PR let's get this info in a comment around https://github.com/openshift-pipelines/pipelines-as-code/blob/0e97e1d1afead542c99ce8004a9f800b0998efe8/pkg/provider/gitlab/gitlab.go#L213 so questions around this do not come up again in a few months and we have to re-research the "why"
savitaashture
commented
4 weeks ago Thank you @chmouel for explaining reason behind not handling the error
@chmouel @gabemontero I have updated the existing comment
PTAL Thank you
savitaashture
commented
4 weeks ago LGTM, was it tested manually as working ? (since there is no E2E)
Yes i did test manually
we need to have a fork repo to test this that's why did not add e2e
In GitLab, when there is an MR from a forked repo to a target repo, it gives a 404 error from the GetMergeRequestChanges function This PR addresses the fix
Signed-off-by: Savita Ashture sashture@redhat.com
Changes
Submitter Checklist
[ ] 📝 Please ensure your commit message is clear and informative. For guidance on crafting effective commit messages, refer to the How to write a git commit message guide. We prefer the commit message to be included in the PR body itself rather than a link to an external website (ie: Jira ticket).
[ ] ♽ Before submitting a PR, run make test lint to avoid unnecessary CI processing. For an even more efficient workflow, consider installing pre-commit and running pre-commit install in the root of this repository.
[ ] ✨ We use linters to maintain clean and consistent code. Please ensure you've run make lint before submitting a PR. Some linters offer a --fix mode, which can be executed with the command make fix-linters (ensure markdownlint and golangci-lint tools are installed first).
[ ] 📖 If you're introducing a user-facing feature or changing existing behavior, please ensure it's properly documented.
[ ] 🧪 While 100% coverage isn't a requirement, we encourage unit tests for any code changes where possible.
[ ] 🎁 If feasible, please check if an end-to-end test can be added. See README for more details.
[ ] 🔎 If there's any flakiness in the CI tests, don't necessarily ignore it. It's better to address the issue before merging, or provide a valid reason to bypass it if fixing isn't possible (e.g., token rate limitations).