-
Notifications
You must be signed in to change notification settings - Fork 1.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: gitlab - ensure lastpipeline ref is the same as head branch #5259
base: main
Are you sure you want to change the base?
feat: gitlab - ensure lastpipeline ref is the same as head branch #5259
Conversation
4cd2848
to
807c10f
Compare
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. |
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]>
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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