Gitlab integration #44

juanjovazquez · 2017-10-17T09:05:13Z

closes #43

codecov-io · 2017-10-17T09:18:31Z

Codecov Report

Merging #44 into master will increase coverage by 1.56%.
The diff coverage is 13.92%.

@@            Coverage Diff             @@
##           master      #44      +/-   ##
==========================================
+ Coverage   58.52%   60.08%   +1.56%     
==========================================
  Files         134      130       -4     
  Lines        4330     4224     -106     
  Branches      107      121      +14     
==========================================
+ Hits         2534     2538       +4     
+ Misses       1796     1686     -110

Impacted Files	Coverage Δ
core/src/main/scala/Github.scala	`26.59% <ø> (ø)`	⬆️
core/src/main/scala/audit/Auditor.scala	`81.81% <ø> (ø)`	⬆️
core/src/main/scala/audit/AuditEvent.scala	`100% <ø> (ø)`	⬆️
core/src/main/scala/Json.scala	`0% <0%> (ø)`	⬆️
core/src/main/scala/Gitlab.scala	`0% <0%> (ø)`
core/src/main/scala/Repo.scala	`50% <0%> (-23.34%)`	⬇️
core/src/main/scala/Released.scala	`100% <100%> (ø)`	⬆️
core/src/main/scala/storage/h2.scala	`82.56% <100%> (ø)`	⬆️
http/src/main/scala/plans/Auth.scala	`96.42% <100%> (ø)`	⬆️
core/src/main/scala/User.scala	`100% <100%> (ø)`	⬆️
... and 12 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1bf8a4d...ff26ba4. Read the comment docs.

timperrett · 2017-12-27T06:07:20Z

@juanjovazquez what's the status of this PR? I would like to label it with a status for admin purposes :-)

juanjovazquez · 2017-12-27T08:50:19Z

@timperrett It's almost all covered except releases and webhooks. This week I'll spend some time trying to get these two aspects finished but, as I mentioned in the issue, the releases part changes quite a bit with respect to Github.

My goal so far has been not to be very disruptive with respect to the current code in order to have a first version working as soon as possible. Later, algebra and interpreters could be refactored to better accommodate the three most important scms: Github, Gitlab and Bitbucket.

If you prefer, we could merge after rebasing as it is now in order to tackle what is missing in individual issues. This PR starts to be too big. Please, let me know what works better for you.

juanjovazquez · 2017-12-27T16:23:30Z

@timperrett webhooks client api implemented. However, I bumped into another drawback: Gitlab's hooks won't set the User-Agent header. Reported here and looking for a workaround. 😠

juanjovazquez · 2017-12-27T17:50:00Z

@timperrett Definitely stuck with the release part. Basically, Gitlab links the concept of release completely to the tag. There is no way to identify the release by itself. And the worst thing is that tags don't have a numerical identifier, but basically the reference as a String, e.g. refs/tags/v1.0.0.

So, my plan is to modify the id type for ReleaseEvent and Release to String. Done this, I'd listen for tags creation events and check if this taghas release notes. In that case, I'd consider it a release.

Quite hacky, but to be honest, I can't find a better way to accommodate this requirement without tackling a profound refactoring. So, if you agree, I'll implement it this way, at least in a first version.

timperrett · 2017-12-27T23:11:49Z

@juanjovazquez is there the ability to attach files to a release / tag in GitLab? That's going to be really important so hope it is available (otherwise this might be moot).

@adelbertc @kaiserpelagic can you just chime in with anything you can think of that might cause a problem with releases having a string ID? Provided they never collide then it should be fine. The only drawback might be table indexing, but I dont recall off the top of my head which columns are indexed; im thinking its not a huge problem.

juanjovazquez · 2017-12-28T10:35:26Z

@timperrett It's a bit hacky but yes, Gitlab does offer the capability of attaching files to tags as release notes through the web interface. It is hacky because Gitlab just uploads the file generating an GUID for it and adds the url as a yaml comment to the description box. So, it's feasible to retrieve this url from the tag info as release notes. An excerpt of the tag info fetched from the API (see the release section, it's the only we have):

  {
    "commit": {
      "id": "2695effb5807a22ff3d138d593fd856244e155e7",
      "short_id": "2695effb",
      "title": "Initial commit",
      "created_at": "2017-07-26T11:08:53.000+02:00",
      "parent_ids": [
        "2a4b78934375d7f53875269ffd4f45fd83a84ebe"
      ],
      "message": "Initial commit",
      "author_name": "John Smith",
      "author_email": "[email protected]",
      "authored_date": "2012-05-28T04:42:42-07:00",
      "committer_name": "Jack Smith",
      "committer_email": "[email protected]",
      "committed_date": "2012-05-28T04:42:42-07:00"
    },
    "release": {
      "tag_name": "1.0.0",
      "description": "[foo.json](/uploads/<guid>/foo.json)"
    },
    "name": "v1.0.0",
    "message": null
  }

When there's no release, the release section in the json is null. So, at least in theory we might identify whether the creation of some tag involves a release or not.

As for the asset info. I won't be able to provide ID's or other fields like name or state. Just the url and of course the content. I might use dummy values for the rest of fields that are not available.

Anyway, I'd still need to change the ID type for releases in order to use tags with release notes as Nelson releases.

juanjovazquez · 2017-12-28T16:37:31Z

core/src/main/resources/db/migration/V2_0__init.sql

@@ -116,7 +116,7 @@ ALTER TABLE PUBLIC."deployments" ADD CONSTRAINT PUBLIC.CONSTRAINT_F PRIMARY KEY(
 CREATE INDEX PUBLIC."deployments_guid_idx" ON PUBLIC."deployments"("guid");    
 CREATE CACHED TABLE PUBLIC."audit_log"(
    "id" BIGINT DEFAULT (NEXT VALUE FOR PUBLIC.SYSTEM_SEQUENCE_15FD08D0_DDC1_4ECB_8FB3_38AB48CE23B4) NOT NULL NULL_TO_DEFAULT SEQUENCE PUBLIC.SYSTEM_SEQUENCE_15FD08D0_DDC1_4ECB_8FB3_38AB48CE23B4,
-    "release_id" BIGINT,
+    "release_id" VARCHAR(25),


AFAICS this is the only change affecting de data model so far as release_id was already defined as VARCHAR(25) in the releases table

@juanjovazquez we should never be changing an existing migration. If you want to change a DB scheme you should create a new migration. Flyaway uses a numbering convention in the name, so you'd create a filed called V2_1__alter_relase_id.sql where you alter the "audit_log" table.

@kaiserpelagic Got it. I'll change it right away.

@juanjovazquez it seems like we also need a migration for the releases table.

@kaiserpelagic Why is that?. release_id was already a VARCHAR(25) in the releases table.

@juanjovazquez interesting, I did not realize that

@kaiserpelagic yep, I got surprise too ;-)

juanjovazquez · 2017-12-28T16:39:01Z

core/src/main/scala/AccessToken.scala

@@ -17,5 +17,6 @@
 package nelson

 final case class AccessToken(
-  value: String
+  value: String,
+  isPrivate: Boolean = false


This is because Gitlab treats OAuth and private personal tokens differently.

Should this be a sum type instead?

sealed abstract class Token extends Product with Serializable object Token { final case class PersonalToken(...) extends Token final case class OAuth(...) extends Token }

juanjovazquez · 2017-12-28T16:42:35Z

core/src/main/scala/Config.scala

-      organizationBlacklist = cfg.lookup[List[String]]("organization-blacklist").getOrElse(Nil),
-      organizationAdminList = cfg.lookup[List[String]]("organization-admins").getOrElse(Nil)
+  private def readGithub(cfg: KConfig): ScmConfig = {
+    val service = cfg.lookup[String]("service").getOrElse("github")


The service configuration param is used to choose the scm interpreter: Gitlab or GIthub (default).

For reading configuration for different interpreters I might do something like

kfg.lookup[String]("scm") match { case Some("github") => readGithub(kfg.subconfig("github")) case Some("gitlab") => readGitlab(kfg.subconfig("gitlab")) case _ => None }

For the time being it's the same configuration.

juanjovazquez · 2017-12-28T16:49:12Z

@timperrett I've updated the PR modifying the Id type for Release and ReleaseEvent. Please, tell me whether you're comfortable with this change so that I can finish the implementation. At first sight the change doesn't seem problematic but maybe I missed something.

kaiserpelagic · 2017-12-28T17:01:38Z

core/src/main/scala/Config.scala

@@ -395,7 +471,12 @@ object Config {
    val nomadcfg = readNomad(cfg.subconfig("nelson.nomad"))

    val gitcfg = readGithub(cfg.subconfig("nelson.github"))


Is configuration of github and gitlab under nelson.github

yep, for the time being. Which one is used is determined by the param service, e.g.

service = "gitlab"

Default is github anyway.

I tried not to add many breaking changes in this first version of Gitlab support, but I think that we should re-think the algebra and the configuration to be more generic. Probably in another PR if you agree.

kaiserpelagic · 2017-12-28T17:03:03Z

core/src/main/scala/Config.scala

@@ -395,7 +471,12 @@ object Config {
    val nomadcfg = readNomad(cfg.subconfig("nelson.nomad"))

    val gitcfg = readGithub(cfg.subconfig("nelson.github"))
-    val git = new Github.GithubHttp(gitcfg, http0)
+    val git: GithubOp ~> Task = gitcfg match {


We should use something other than GithubOp now

Same idea as before. I left that kind of changes for a second review.

kaiserpelagic · 2017-12-28T17:16:09Z

core/src/main/scala/Gitlab.scala

+
+      case GetUserRepositories(t: AccessToken) =>
+        def go(uri: String)(accum: List[Repo]): List[Repo] = {
+          val EntityWithMeta(repos, pagination) = authFetchWithMeta[List[Repo]](uri, t).run


We should never be calling .run in code like this.

yeah, it's basically the same thing as in the Github interpreter. I'll have it a look anyway.

kaiserpelagic · 2018-01-11T17:10:59Z

core/src/main/scala/Json.scala

@@ -505,12 +509,22 @@ object Json {
      e <- (z --\ "repository" --\ "id").as[Long]
    } yield {
      Github.ReleaseEvent(
-        id = a,
+        id = a.toString,


Why not decode it as a String above, instead of calling toString here?

Sure, take it for granted.

kaiserpelagic · 2018-01-11T17:13:42Z

core/src/main/scala/Released.scala

@@ -30,7 +30,7 @@ final case class Released(
  /* when was this release created */
  timestamp: Instant,
  /* reference id from github */
-  releaseId: Long,
+  releaseId: String,


Why is this a String now? does Gitlab not represent it was a number?

Unfortunately, it's not the case. Gitlab has no releases, just tags, and these have no numeric ids either. 😞

adelbertc

Did you want to merge this as is or just put it up for review? I think it'd also be good to get changes that others suggested in, like renaming GithubOp to something more generic, etc.

Perhaps those can be made in a separate commit so it's easy to revert if you're concerned about touching too many things.

adelbertc · 2018-01-23T15:32:15Z

core/src/main/resources/db/migration/V2_1__alter_release_id.sql

+--:   limitations under the License.
+--:
+--: ----------------------------------------------------------------------------
+ALTER TABLE PUBLIC."audit_log" ALTER COLUMN "release_id" VARCHAR(25);


I am not familiar with DB migrations, this will probably need @timperrett 's eyes.

adelbertc · 2018-01-23T15:33:25Z

core/src/main/scala/AccessToken.scala

@@ -17,5 +17,6 @@
 package nelson

 final case class AccessToken(
-  value: String
+  value: String,
+  isPrivate: Boolean = false


Should this be a sum type instead?

sealed abstract class Token extends Product with Serializable object Token { final case class PersonalToken(...) extends Token final case class OAuth(...) extends Token }

adelbertc · 2018-01-23T15:36:05Z

core/src/main/scala/Config.scala

+      case _: ScmConfig.GithubConfig =>
+        new Github.GithubHttp(gitcfg, http0)
+      case _ =>
+        new Gitlab.GitlabHttp(gitcfg, http4sClient(timeout))


Picky but I generally prefer pat mat instead of type casing, if only for theoretical reasons described here: https://typelevel.org/blog/2014/11/10/why_is_adt_pattern_matching_allowed.html

Also perhaps get rid of _ and do an explicit match on GitLab's constructor so any potential future additions error here as they should.

adelbertc · 2018-01-23T15:39:49Z

core/src/main/scala/Github.scala

@@ -226,7 +226,7 @@ object Github {
    }
  }

-  final class GithubHttp(cfg: GithubConfig, http: dispatch.Http) extends (GithubOp ~> Task) {
+  final class GithubHttp(cfg: ScmConfig, http: dispatch.Http) extends (GithubOp ~> Task) {


mmmmm this is a bit bothersome. On one hand I don't like using subtypes for data types that are ADTs on the other hand the only config this should be called with is the GithubConfig.. I would learn towards using the subtype anyways.

adelbertc · 2018-01-23T15:40:48Z

core/src/main/scala/Github.scala

@@ -76,13 +76,13 @@ object Github {
  ) extends Event

  final case class ReleaseEvent(
-    id: Long,
+    id: String,


One trick we can try is parameterize it into ReleaseEvent[A] so we have ReleaseEvent[Long] and ReleaseEvent[String].. wdyt? @timperrett @kaiserpelagic

adelbertc · 2018-01-23T15:41:52Z

core/src/main/scala/Json.scala

+    DecodeJson(c => (c --\ "access_token").as[String]).map(AccessToken.apply(_))
+
+  implicit lazy val AccessTokenEncoder: EncodeJson[AccessToken] =
+    jencode1L((t: AccessToken) => t.value)("access_token")


Why this change?

juanjovazquez force-pushed the wip-gitlab-support branch from d0b2131 to 9f41949 Compare December 4, 2017 09:54

juanjovazquez force-pushed the wip-gitlab-support branch from 9f41949 to 1fd71a9 Compare December 11, 2017 12:32

juanjovazquez force-pushed the wip-gitlab-support branch 2 times, most recently from cb5273d to 3060b3b Compare December 20, 2017 09:01

timperrett added the control:unmergable label Dec 27, 2017

juanjovazquez commented Dec 28, 2017

View reviewed changes

kaiserpelagic reviewed Dec 28, 2017

View reviewed changes

juanjovazquez force-pushed the wip-gitlab-support branch from 4ce3c4b to e25dd68 Compare January 10, 2018 17:58

juanjovazquez changed the title ~~WIP Gitlab integration~~ Gitlab integration Jan 10, 2018

kaiserpelagic reviewed Jan 11, 2018

View reviewed changes

juanjovazquez force-pushed the wip-gitlab-support branch 2 times, most recently from ab9f2e9 to 61158e6 Compare January 15, 2018 11:41

Implements Gitlab integration (fixes getnelson#43)

ff26ba4

juanjovazquez force-pushed the wip-gitlab-support branch from 61158e6 to ff26ba4 Compare January 22, 2018 09:06

adelbertc requested changes Jan 23, 2018

View reviewed changes

		@@ -395,7 +471,12 @@ object Config {
		val nomadcfg = readNomad(cfg.subconfig("nelson.nomad"))

		val gitcfg = readGithub(cfg.subconfig("nelson.github"))

Gitlab integration #44

Are you sure you want to change the base?

Gitlab integration #44

Conversation

juanjovazquez commented Oct 17, 2017

codecov-io commented Oct 17, 2017 • edited Loading

Codecov Report

timperrett commented Dec 27, 2017

juanjovazquez commented Dec 27, 2017

juanjovazquez commented Dec 27, 2017

juanjovazquez commented Dec 27, 2017

timperrett commented Dec 27, 2017

juanjovazquez commented Dec 28, 2017

Choose a reason for hiding this comment

kaiserpelagic Dec 28, 2017 • edited Loading

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

kaiserpelagic Dec 28, 2017 • edited Loading

Choose a reason for hiding this comment

juanjovazquez Dec 28, 2017 • edited Loading

Choose a reason for hiding this comment

juanjovazquez commented Dec 28, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adelbertc 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

codecov-io commented Oct 17, 2017 •

edited

Loading

kaiserpelagic Dec 28, 2017 •

edited

Loading

kaiserpelagic Dec 28, 2017 •

edited

Loading

juanjovazquez Dec 28, 2017 •

edited

Loading