Adding support for Gitlab #149
Conversation
Prsna23
commented
Aug 16, 2020
Prsna23
commented
- Adding gitlab pr plugin to trigger builds for GitLab merge requests.
src/main/java/in/ashwanthkumar/gocd/github/provider/gitlab/GitLabProvider.java
Show resolved
Hide resolved
src/main/resources/gitlab/plugin.xml
Outdated
| <about> | ||
| <name>Gitlab Pull Requests Builder</name> | ||
| <version>${version}</version> | ||
| <target-go-version>15.1.0</target-go-version> |
There was a problem hiding this comment.
You might want to update this to a more latest version since you're using more latest JDK dependency :)
There was a problem hiding this comment.
Sure will update the version.
src/main/resources/gitlab/plugin.xml
Outdated
| <name>Ashwanth Kumar</name> | ||
| <url>https://github.com/ashwanthkumar/gocd-build-github-pull-requests</url> |
There was a problem hiding this comment.
Please consider adding your name to this. This is your work and you deserve the credit!
src/main/java/in/ashwanthkumar/gocd/github/provider/gitlab/GitLabProvider.java
Outdated
Show resolved
Hide resolved
|
|
||
| PullRequestStatus prStatus = null; | ||
| boolean isDisabled = System.getProperty("go.plugin.gitlab.pr.populate-details", "Y").equals("N"); | ||
| LOG.debug("Populating PR details is disabled"); |
There was a problem hiding this comment.
This log should be inside a if (isDisabled) block isn't?
There was a problem hiding this comment.
Yes will change it 👍
src/main/java/in/ashwanthkumar/gocd/github/provider/gitlab/GitLabProvider.java
Outdated
Show resolved
Hide resolved
src/main/java/in/ashwanthkumar/gocd/github/provider/gitlab/GitLabProvider.java
Outdated
Show resolved
Hide resolved
src/main/java/in/ashwanthkumar/gocd/github/provider/gitlab/GitLabUtils.java
Outdated
Show resolved
Hide resolved
src/main/java/in/ashwanthkumar/gocd/github/provider/gitlab/GitLabProvider.java
Outdated
Show resolved
Hide resolved
src/test/java/in/ashwanthkumar/gocd/github/provider/gitlab/GitLabUtilsTest.java
Outdated
Show resolved
Hide resolved
| gitConfig.setPassword(props.getProperty(PASSWORD)); | ||
| } | ||
| } catch (IOException e) { | ||
| // ignore |
There was a problem hiding this comment.
Please always throw an exception so it bubbles up on the Server Dashboard with the error Message. Wrap all exceptions inside a RuntimeException.
| public void addConfigData(GitConfig gitConfig) { | ||
| try { | ||
| Properties props = GitLabUtils.readPropertyFile(); | ||
| if (StringUtil.isEmpty(gitConfig.getUsername())) { | ||
| gitConfig.setUsername(props.getProperty(LOGIN)); | ||
| } | ||
| if (StringUtil.isEmpty(gitConfig.getPassword())) { | ||
| gitConfig.setPassword(props.getProperty(PASSWORD)); | ||
| } |
There was a problem hiding this comment.
Can you also please update the README with the format of the config file and how to setup the plugin?
|
I think apart the above things, we're good to merge this into master and cut a release! |
Gitlab4j-api requires jackson version >= 2.9.3. Reference:gitlab4j/gitlab4j-api#184
| LOG.warn(e.getMessage(), e); | ||
| } return null; | ||
| throw new RuntimeException(String.format("Failed to fetch PR status. %s", e.getMessage()), e); |
There was a problem hiding this comment.
It would be nice to have the log and error have the same message so it is easier to grep on the logs when needed.
| @@ -65,7 +64,7 @@ public boolean isValidURL(String url) { | |||
| @Override | |||
| public void checkConnection(GitConfig gitConfig) { | |||
| try { | |||
| loginWith(gitConfig).getProjectApi().getProjects(); | |||
| loginWith(gitConfig).getProjectApi().getProject(GHUtils.parseGithubUrl(gitConfig.getEffectiveUrl())); | |||
| } catch (Exception e) { | |||
| throw new RuntimeException(String.format("check connection failed. %s", e.getMessage()), e); | |||
There was a problem hiding this comment.
Please consider logging the exception before throwing them.
| } catch (IOException e) { | ||
| // ignore | ||
| } catch (Exception e) { | ||
| throw new RuntimeException(String.format("Adding config data failed. %s", e.getMessage()), e); |
There was a problem hiding this comment.
Please consider logging the exception before throwing them.
There was a problem hiding this comment.
@Prsna23 LGTM. Can I merge these changes and cut out a beta release?
|
Yes please 😃 |
|
We might need some changes on https://github.com/ashwanthkumar/gocd-build-github-pull-requests/blob/master/release.sh. Not a blocker, I can also do these changes as part of merging it to master. Just putting it out there. |
|
I think I have edited the |
|
Ah, I missed that somehow. Thanks. Merging it now. |
