-
-
Notifications
You must be signed in to change notification settings - Fork 398
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
cmd: use CI_API_V4_URL env as gitlab API URL #565
Conversation
cmd/reviewdog/main.go
Outdated
baseURL := os.Getenv("GITLAB_API") | ||
if baseURL == "" { | ||
gitlabApi := os.Getenv("GITLAB_API") | ||
gitlabV4Url := os.Getenv("CI_API_V4_URL") |
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.
gitlabV4URL
.
Also, please fix the above golint error as well.
var gitlabApi should be gitlabAPI
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.
Done
cmd/reviewdog/main.go
Outdated
gitlabV4Url := os.Getenv("CI_API_V4_URL") | ||
|
||
var baseURL string | ||
switch true { |
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 you use if-else clause? It's interesting, but simple if-else should be better.
Alternatively, you can create another function which returns URL string and use early return.
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 thought about the function first, but then it seemed weird to me to have the current gitlabBaseURL to just be a wrapper for url.Parse
. I have switched to the if-else chain.
Also, can you update Unreleased section in the changelog as well? https://github.com/reviewdog/reviewdog/blob/master/CHANGELOG.md |
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.
Thanks!
This should be a simple change, although it has not been extensively tested.
Closes #563.