feat: gitlab - ensure lastpipeline ref is the same as head branch #5259
Conversation
rhariady
requested review from
GenPage,
lukemassa and
X-Guardian
and removed request for
a team
rhariady
force-pushed
the
fix-5228-gitlab-pipeline-status
branch
from
4cd2848 to
807c10f
Compare
rhariady
force-pushed
the
fix-5228-gitlab-pipeline-status
branch
from
0daeea2 to
fd84a2b
Compare
|
Hi @rhariady, I don't think this is going to totally fix the issue, as if the ref does not match, it is going to retry then fall back to using I think this can be resolved by adding some more code when the ref does not match to then use the list-the-status-of-a-commit API to find the correct status to update. |
X-Guardian
added
waiting-on-response
Signed-off-by: Ricky Hariady <[email protected]>
Signed-off-by: Ricky Hariady <[email protected]>
Signed-off-by: Ricky Hariady <[email protected]>
Signed-off-by: Ricky Hariady <[email protected]>
rhariady
force-pushed
the
fix-5228-gitlab-pipeline-status
branch
from
9648c1d to
4636a84
Compare
|
Hi @X-Guardian! I have pushed another commit that implement your suggestion, but instead of calling list-the-status-of-a-commit only when the ref doesn't match, I end up using it to replace call to get-a-single-commit API in the first place. I think it will be more effective, because we will have 1 less API call for every retry (in case there are another pipeline running). Also, i think list-the-status-of-a-commit is more aligned with the subsequent call, which is to set-the-status-of-a-commit. |
Signed-off-by: Ricky Hariady <[email protected]>
| var resp *gitlab.Response | ||
| var err error | ||
|
|
||
| // get the last commit status with the same ref | ||
| getCommitStatusesOptions := &gitlab.GetCommitStatusesOptions{ |
There was a problem hiding this comment.
Can we have better handling on this if the function returns multiple results? Forcing the PerPage to 1 is pretty ugly.
If we are not expecting more than one result, we should be checking the length and making a decision on what we want to do in that scenario. i.e. using the first result, but at least report a warning that more than one result was returned, or just erroring.
There was a problem hiding this comment.
Hi @X-Guardian! Sorry i just got back now after a while.
It's actually very possible that the response contain multiple results, in case the pipeline contain multiple job, but all should have identical PipelineID because for each commit there should only one pipeline per ref (gitlab will screaming error when creating new pipeline with same ref for single commit). So, in the end it doesn't matter which job (element) of the list to use.
On contrary, in existing implementation, we sometime mixed between pipeline from different ref. By filtering with specific ref, we can prevent this mixing from happened.
There was a problem hiding this comment.
So if this is the case, rather than making the assumption, the code should verify that.
what
This MR will change the behaviour of GitlabClient UpdateStatus to use previous lastPipeline only if it has a same Ref (branch).
why
This is to fix #5228 where a commit is referenced as head of two different branch (in case someone duplicate a branch and create separate MR)
tests
references