feat(gitlab): Add Gitlab SCM integration (spinnaker/spinnaker#2047) #197

mdirkse · 2017-11-20T15:51:51Z

Enables Igor scm functionality for GitLab (as per spinnaker/spinnaker#2047)

gardleopard · 2017-11-21T13:05:01Z

igor-web/src/main/groovy/com/netflix/spinnaker/igor/config/GitLabProperties.groovy

+import javax.validation.constraints.NotNull
+
+/**
+ * Helper class to map masters in properties file into a validated property map


I think this comment is a bit misleading, what is masters in this context?

More nitpick: It's not really a map, it's just a standard type-safe Spring Boot config.

As the GitLab implementation is pretty similar to the GitHub one we just copied this comment from https://github.com/spinnaker/igor/blob/master/igor-web/src/main/groovy/com/netflix/spinnaker/igor/config/GitHubProperties.groovy#L27
Should we omit it?

gardleopard · 2017-11-21T13:20:41Z

igor-web/src/main/groovy/com/netflix/spinnaker/igor/scm/gitlab/client/GitLabClient.groovy

@@ -0,0 +1,22 @@
+package com.netflix.spinnaker.igor.scm.gitlab.client


missing license.

gardleopard · 2017-11-21T13:21:26Z

igor-web/src/main/groovy/com/netflix/spinnaker/igor/scm/gitlab/client/GitLabClient.groovy

+import retrofit.http.QueryMap
+
+/**
+ * Interface for interacting with a Gitab REST API


Typo Gitab -> GitLab.

gardleopard

Thank you for the contribution! I have made some comments on small things that needs to be fixed.

gardleopard · 2017-11-21T13:24:15Z

igor-web/src/main/groovy/com/netflix/spinnaker/igor/scm/gitlab/client/GitLabMaster.groovy

+import groovy.util.logging.Slf4j
+
+/**
+ * Wrapper class for a collection of GitLab clients


Can this ever be a collection? It looks like a single gitlab integration can be defined in the configuration. It looks more like Wrapper for the gitlab client.

Also copied from the GitHub variant.

gardleopard · 2017-11-21T13:24:34Z

igor-web/src/main/groovy/com/netflix/spinnaker/igor/scm/gitlab/client/model/Commit.groovy

@@ -0,0 +1,19 @@
+package com.netflix.spinnaker.igor.scm.gitlab.client.model


Missing license.

gardleopard · 2017-11-21T13:24:49Z

...main/groovy/com/netflix/spinnaker/igor/scm/gitlab/client/model/CompareCommitsResponse.groovy

@@ -0,0 +1,8 @@
+package com.netflix.spinnaker.igor.scm.gitlab.client.model


Missing license.

jervi · 2017-11-21T13:11:17Z

igor-web/src/main/groovy/com/netflix/spinnaker/igor/config/GitLabProperties.groovy

+import javax.validation.constraints.NotNull
+
+/**
+ * Helper class to map masters in properties file into a validated property map


More nitpick: It's not really a map, it's just a standard type-safe Spring Boot config.

jervi · 2017-11-21T13:16:02Z

igor-web/src/main/groovy/com/netflix/spinnaker/igor/scm/gitlab/CommitController.groovy

+        try {
+            commitsResponse = master.gitLabClient.getCompareCommits(projectKey, repositorySlug, [to: requestParams.from, from: requestParams.to])
+        } catch (RetrofitError e) {
+            if(e.getKind() == RetrofitError.Kind.NETWORK) {


space after if

jervi · 2017-11-21T13:16:13Z

igor-web/src/main/groovy/com/netflix/spinnaker/igor/scm/gitlab/CommitController.groovy

+        } catch (RetrofitError e) {
+            if(e.getKind() == RetrofitError.Kind.NETWORK) {
+                throw new RuntimeException("Could not find the server ${master.baseUrl}")
+            } else if(e.response.status == 404) {


space after if

jervi · 2017-11-21T13:16:33Z

igor-web/src/main/groovy/com/netflix/spinnaker/igor/scm/gitlab/CommitController.groovy

+
+        commitsResponse.commits.each {
+            def commitUrl = getCommitUrl(gitLabProperties.baseUrl, projectKey, repositorySlug, it?.id)
+            result << [displayId: it?.id?.substring(0,gitLabProperties.commitDisplayLength), id: it?.id, authorDisplayName: it?.author_name,


space after 0,

jervi · 2017-11-21T13:18:40Z

igor-web/src/main/groovy/com/netflix/spinnaker/igor/scm/gitlab/client/GitLabClient.groovy

+    /**
+     * https://docs.gitlab.com/ce/api/repositories.html#compare-branches-tags-or-commits
+     */
+    @GET('/api/v4/projects/{projectKey}%2F{repositorySlug}/repository/compare')


You can't use / instead of %2F?

Not really, because Gitlab API expects a URL-encoded project id:

id (required) - The ID or URL-encoded path of the project owned by the authenticated user

(from https://docs.gitlab.com/ce/api/repositories.html#compare-branches-tags-or-commits)

jervi · 2017-11-21T13:23:45Z

igor-web/src/main/groovy/com/netflix/spinnaker/igor/scm/gitlab/client/model/Commit.groovy

+import com.fasterxml.jackson.annotation.JsonIgnoreProperties
+import com.fasterxml.jackson.annotation.JsonProperty
+import org.joda.time.DateTime
+import org.joda.time.format.ISODateTimeFormat


I would strongly consider to use Java 8 Time & Date API (JSR 310) instead of Joda Time. I think Jackson converters are already included on the class path. If not, just include compile spinnaker.dependency('jacksonJsr310') in the gradle config.

JodaTime is also used in the code that talks to BitBucket. In the interest of consistency we decided to use it here as well. Perhaps we could do a separate PR to rid the project of JodaTime al together?

I'm fine with that 👍

jervi · 2017-11-21T13:26:53Z

igor-web/src/test/groovy/com/netflix/spinnaker/igor/scm/gitlab/client/GitLabClientSpec.groovy

+    }
+
+    String getCompareCommitsResponse() {
+        return '{\n' +


Groovy has multiline support. See http://groovy-lang.org/syntax.html#_triple_single_quoted_string

lwander · 2017-11-30T13:15:02Z

I don't know how necessary this is with the artifact support. I'm guessing we are just relying on webhooks?

mdirkse · 2017-11-30T22:44:53Z

Alright, fixed all review remarks and redid the whole thing in Java. Which mean that I also had to port the AbstractCommitController due to compilation ordering issues. So I ended up touching some other stuff that's not strictly gitlab related.

jervi

Thanks for the fixes! I have some minor comments, primarily regarding the Java conversion. Otherwise I think it looks good, disregarding the comment from @lwander.
Lars: you mean no more native SCM integrations should be created?
Edit: I just saw the discussion on Slack #dev.

jervi · 2017-12-01T09:36:13Z

igor-web/src/main/java/com/netflix/spinnaker/igor/config/GitLabProperties.java

+    public String privateToken;
+
+    @NotNull
+    public Integer commitDisplayLength;


@ConfigurationProperties-classes should be standard POJO's, so make these three fields private and create getters

jervi · 2017-12-01T09:39:49Z

igor-web/src/main/java/com/netflix/spinnaker/igor/scm/AbstractCommitController.java

+        eMap.put("displayId", "NOT_FOUND");
+        eMap.put("id", "NOT_FOUND");
+        eMap.put("authorDisplayName", "UNKNOWN");
+        eMap.put("timestamp", new Date(0));


Use JSR310 (or Joda Time, as discussed earlier). For instance Instant.now().

jervi · 2017-12-01T09:41:54Z

igor-web/src/main/java/com/netflix/spinnaker/igor/scm/gitlab/CommitController.java

+    private final GitLabMaster gitLabMaster;
+    private final GitLabProperties gitLabProperties;
+
+    public CommitController(@Autowired GitLabMaster gitLabMaster, @Autowired GitLabProperties gitLabProperties) {


Just put the @Autowired annotation above the constructor, and remove the ones in front of the parameters.

jervi · 2017-12-01T11:06:35Z

igor-web/src/main/java/com/netflix/spinnaker/igor/scm/AbstractCommitController.java

+
+
+    @ResponseStatus(value = HttpStatus.BAD_REQUEST, reason = "toCommit and fromCommit parameters are required in the query string")
+    @InheritConstructors


@InheritConstructors is groovy-stuff, so remove it

jervi · 2017-12-01T11:18:00Z

igor-web/src/main/java/com/netflix/spinnaker/igor/scm/AbstractCommitController.java

+
+    @RequestMapping(value = "/{projectKey}/{repositorySlug}/compareCommits")
+    public List<Map<String, Object>> compareCommits(@PathVariable(value = "projectKey") String projectKey,
+                                                    @PathVariable(value = "repositorySlug") String repositorySlug,


You can remove the annotation parameter, Spring will default to using the parameter name as the value. E.g.: @PathVariable String projectKey

jervi · 2017-12-01T11:26:05Z

igor-web/src/main/java/com/netflix/spinnaker/igor/scm/gitlab/client/model/Commit.java

+    public final String message;
+
+    @JsonCreator
+    public Commit(@JsonProperty("id") String id,


You can remove the annotation value for id and message, as Jackson will default to the field name. Leave it for author_name and authored_date (if you rename the fields, that is).

Jackson gets confused if I remove them.

jervi · 2017-12-01T11:26:19Z

igor-web/src/main/java/com/netflix/spinnaker/igor/scm/gitlab/client/model/Commit.java

+    @JsonCreator
+    public Commit(@JsonProperty("id") String id,
+                  @JsonProperty("author_name") String author_name,
+                  @JsonProperty("authored_date") Date authored_date,


jervi · 2017-12-01T11:26:46Z

...src/main/java/com/netflix/spinnaker/igor/scm/gitlab/client/model/CompareCommitsResponse.java

+    public final List<Commit> commits;
+
+    @JsonCreator
+    public CompareCommitsResponse(@JsonProperty("commits") List<Commit> commits) {


You can remove the annotation value

Can't, Jackson gets confused if I do

Yeah, you are correct. The JavaDoc on @JsonProperty seems to suggest that it's not needed, but the documentation on @JsonCreator is quite explicit on it being needed in plain Java code.

jervi · 2017-12-01T11:28:00Z

igor-web/src/main/java/com/netflix/spinnaker/igor/scm/gitlab/client/GitLabMaster.java

+ */
+public class GitLabMaster {
+    public final GitLabClient gitLabClient;
+    public final String baseUrl;


Make these private and create getters.

jervi · 2017-12-01T11:28:49Z

igor-web/src/main/java/com/netflix/spinnaker/igor/scm/AbstractCommitController.java

+    protected ExecutorService executor;
+    protected ObjectMapper objectMapper;
+
+


Remove one of the blank lines

jervi · 2017-12-01T11:35:47Z

igor-web/src/main/java/com/netflix/spinnaker/igor/scm/AbstractCommitController.java

+
+    @ResponseStatus(value = HttpStatus.BAD_REQUEST, reason = "toCommit and fromCommit parameters are required in the query string")
+    @InheritConstructors
+    public static class MissingParametersException extends RuntimeException {


This can be private, it's not used anywhere else. But it might make sense to keep it public for future use though.

mdirkse · 2017-12-04T13:07:58Z

@jervi fixed all things that could be fixed, thanks for the review!

jervi

LGTM

jervi · 2017-12-06T11:07:56Z

igor-web/src/test/groovy/com/netflix/spinnaker/igor/scm/gitlab/client/GitLabClientSpec.groovy

@@ -0,0 +1,133 @@
+package com.netflix.spinnaker.igor.scm.gitlab.client


Missing license

gardleopard · 2017-12-06T11:12:11Z

@lwander this is useful for doing the diff calculation in the deployment stages.

gardleopard · 2017-12-06T13:16:56Z

We are running this in our test environment now and our deployments still gives us the change list, so this looks all good to me.

gardleopard · 2017-12-06T13:26:51Z

@tomaslin can you take a look at this?

jtk54

LGTM.

jtk54 · 2017-12-06T13:54:03Z

igor-web/src/main/java/com/netflix/spinnaker/igor/config/GitLabConfig.java

+
+    @Bean
+    public GitLabMaster gitLabMasters(@Valid GitLabProperties gitLabProperties) {
+        log.info("bootstrapping {} as gitlab", gitLabProperties.getBaseUrl());


s/as gitlab/as gitlab master.

jeyrschabu · 2017-12-09T03:05:18Z

LGTM

mdirkse force-pushed the gitlab_ci_support branch from afce9c3 to 3bdfdb3 Compare November 20, 2017 16:01

wheleph force-pushed the gitlab_ci_support branch from b7a6862 to 488e571 Compare November 21, 2017 12:21

mdirkse force-pushed the gitlab_ci_support branch from 769e9dc to 691371a Compare November 21, 2017 12:46

gardleopard reviewed Nov 21, 2017

View reviewed changes

gardleopard requested changes Nov 21, 2017

View reviewed changes

jervi suggested changes Nov 21, 2017

View reviewed changes

mdirkse force-pushed the gitlab_ci_support branch 2 times, most recently from d422c33 to ca7cdf3 Compare November 30, 2017 22:43

jervi suggested changes Dec 1, 2017

View reviewed changes

jervi reviewed Dec 1, 2017

View reviewed changes

mdirkse force-pushed the gitlab_ci_support branch from ca7cdf3 to 7878b21 Compare December 4, 2017 13:06

jervi approved these changes Dec 6, 2017

View reviewed changes

gardleopard approved these changes Dec 6, 2017

View reviewed changes

mdirkse force-pushed the gitlab_ci_support branch from 7878b21 to 37fff8c Compare December 6, 2017 11:29

feat(gitlab): Add Gitlab SCM integration (spinnaker/spinnaker#2047)

a2840ff

mdirkse force-pushed the gitlab_ci_support branch from 37fff8c to a2840ff Compare December 6, 2017 11:52

jtk54 approved these changes Dec 6, 2017

View reviewed changes

wheleph mentioned this pull request Dec 8, 2017

WIP: feat(gitlab CI integration) (spinnaker/spinnaker#2047) #207

Closed

mdirkse added 2 commits December 10, 2017 21:42

Merge branch 'master' into gitlab_ci_support

beb8685

Merge branch 'master' into gitlab_ci_support

a3b40ec

gardleopard merged commit 4297671 into spinnaker:master Dec 19, 2017

		@@ -0,0 +1,22 @@
		package com.netflix.spinnaker.igor.scm.gitlab.client

		@@ -0,0 +1,19 @@
		package com.netflix.spinnaker.igor.scm.gitlab.client.model

		@@ -0,0 +1,8 @@
		package com.netflix.spinnaker.igor.scm.gitlab.client.model



		@ResponseStatus(value = HttpStatus.BAD_REQUEST, reason = "toCommit and fromCommit parameters are required in the query string")
		@InheritConstructors

		protected ExecutorService executor;
		protected ObjectMapper objectMapper;

		@@ -0,0 +1,133 @@
		package com.netflix.spinnaker.igor.scm.gitlab.client

feat(gitlab): Add Gitlab SCM integration (spinnaker/spinnaker#2047) #197

feat(gitlab): Add Gitlab SCM integration (spinnaker/spinnaker#2047) #197

Conversation

mdirkse commented Nov 20, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gardleopard left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wheleph Nov 30, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lwander commented Nov 30, 2017

mdirkse commented Nov 30, 2017

jervi left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mdirkse commented Dec 4, 2017

jervi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gardleopard commented Dec 6, 2017

gardleopard commented Dec 6, 2017

gardleopard commented Dec 6, 2017

jtk54 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeyrschabu commented Dec 9, 2017

wheleph Nov 30, 2017 •

edited

jervi left a comment •

edited