-
Notifications
You must be signed in to change notification settings - Fork 16
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: add gitlab support #205
Conversation
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.
Looking fantastic! Did a quick first round review.
func (pr *PullRequest) Update(ctx context.Context, title, description string, prObj *v1alpha1.PullRequest) error { | ||
logger := log.FromContext(ctx) | ||
|
||
mrIID, err := strconv.Atoi(prObj.Status.ID) |
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.
I wonder if we should standardize on "Merge Request" within the context of the gitlab-specific code for variable names, etc.
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.
Do you mean that it's currently e.g. repo
vs. gitRepo
?
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.
nah, it just that most stuff in the new gitlab code refers to "pull requests" even though gitlab calls them "merge requests." Not a big deal, but maybe better to stick with the SCM's convention.
One thing to note and this is more information than anything that needs to be done in this PR, there is also webhook support that is somewhat required at lest for faster reconciliation triggering than the default I think 10 minutes. There is not much support for multiple providers but you can see that here |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #205 +/- ##
==========================================
- Coverage 50.94% 50.25% -0.70%
==========================================
Files 15 15
Lines 1751 1793 +42
==========================================
+ Hits 892 901 +9
- Misses 739 773 +34
+ Partials 120 119 -1 ☔ View full report in Codecov by Sentry. |
afb8774
to
6be7e34
Compare
@zachaller good catch, created #222 to keep track of that. |
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.
Let's heckin' go: https://gitlab.com/mac9416/gitops-promoter-tests/-/merge_requests
func (pr *PullRequest) Update(ctx context.Context, title, description string, prObj *v1alpha1.PullRequest) error { | ||
logger := log.FromContext(ctx) | ||
|
||
mrIID, err := strconv.Atoi(prObj.Status.ID) |
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.
nah, it just that most stuff in the new gitlab code refers to "pull requests" even though gitlab calls them "merge requests." Not a big deal, but maybe better to stick with the SCM's convention.
I'm not getting anything for rate limits:
|
Signed-off-by: Robin Lieb <[email protected]>
Signed-off-by: Robin Lieb <[email protected]>
Signed-off-by: Robin Lieb <[email protected]>
Signed-off-by: Robin Lieb <[email protected]>
Co-authored-by: Michael Crenshaw <[email protected]> Signed-off-by: Robin Lieb <[email protected]>
37d4090
to
c3b19a6
Compare
Signed-off-by: Robin Lieb <[email protected]>
As far as I understand they are only provided on GitLab Self-Managed and also only once the limit is reached. There is no information provided to get the current status of the rate limit. I changed the implementation to only log if they are provided in the response. |
Implements #141