RHTAP-1134: Support custom Gitlab instances#240
Conversation
|
@gbenhaim: This pull request references RHTAP-1134 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the feature to target the "4.16.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
gbenhaim
force-pushed
the
gitlab_base_url
branch
from
fd9bfc7 to
03ed9ef
Compare
chmeliik
left a comment
chmeliik
left a comment
There was a problem hiding this comment.
LGTM with a minor nitpick for the test data
| err error | ||
| }{ | ||
| { | ||
| "https://gitlab.cee.redhat.com/rhtap/tenants-config.git", |
There was a problem hiding this comment.
AFAIK we should generally avoid mentioning internal URLs in upstream code, could you replace the redhat.com urls with something else?
When creating a Gitlab client, the base url of the Gitlab instance should be provided (unless the default is used, which is https://gitlab.com). With this change, it would be possible to use Gitlab instance other than gitlab.com. In addition, fixed an issue in "getWebhookByTargetUrl" where it tried to access a nil pointer of a request. Signed-off-by: gbenhaim <gbenhaim@redhat.com>
gbenhaim
force-pushed
the
gitlab_base_url
branch
from
03ed9ef to
fc52140
Compare
|
mmorhun
left a comment
mmorhun
left a comment
There was a problem hiding this comment.
LGTM in general, just a small remark.
Thanks for contributions!
| func GetBaseUrl(repoUrl string) (string, error) { | ||
| url, err := url.Parse(repoUrl) | ||
| if err != nil { | ||
| return "", FailedToParseUrlError{url: repoUrl, err: err.Error()} |
There was a problem hiding this comment.
Custom errors looks redundant here: they are never used / distinguished.
There was a problem hiding this comment.
they are used in the tests :)
|
/retest |
|
@chmeliik can you please merge it? I see that all the checks passed. thanks! |
4 participants
When creating a Gitlab client, the base url of the Gitlab instance should be provided (unless the default is used, which is https://gitlab.com). With this change, it would be possible to use Gitlab instance other than gitlab.com.
In addition, fixed an issue in "getWebhookByTargetUrl" where it tried to access a nil pointer of a request.