add GitLab Release badge, run all [GitLab] #7021

calebcartwright · 2021-09-15T01:38:23Z

Resolves #4867

shields-ci · 2021-09-15T01:40:27Z

	Warnings
⚠️	📚 Remember to ensure any changes to `config.private` in `services/gitlab/gitlab-base.js` are reflected in the server secrets documentation

	Messages
📖	✨ Thanks for your contribution to Shields, @calebcartwright!

Generated by 🚫 dangerJS against f32a86e

calebcartwright · 2021-09-16T01:04:03Z

services/gitlab/gitlab-base.js

+  async fetchPaginatedArrayData({
+    url,
+    options,
+    schema,
+    errorMessages,
+    firstPageOnly = false,
+  }) {
+    const requestParams = this.authHelper.withBasicAuth({
+      url,
+      options: {
+        headers: { Accept: 'application/json' },
+        qs: { per_page: 100 },
+        ...options,
+      },
+      errorMessages,
+    })
+
+    const {
+      res: { headers },
+      data,
+    } = await this.fetchPage({ page: 1, requestParams, schema })
+    const numberOfPages = headers['x-total-pages']
+
+    if (numberOfPages === 1 || firstPageOnly) {
+      return data
+    }
+
+    const pageData = await Promise.all(
+      [...Array(numberOfPages - 1).keys()].map((_, i) =>
+        this.fetchPage({ page: ++i + 1, requestParams, schema })
+      )
+    )
+    return [...data].concat(...pageData)
+  }


GitLab's max per page value is 100, and I saw that exceeded in several cases just with the releases on the handful of projects I tested with. A cursory glance through the GitLab API docs makes it seem a very common response structure will be pages of arrays with the page keys in the response headers.

Opted to go ahead and pull that into the base class in anticipation of other APIs. Added it with relevant short circuiting logic (since GitLab doesn't seem to have a separate "latest" API endpoint that returns a single object like GitHub does in some cases), and also requesting all the pages in parallel since it's both possible to do so and preferable to 1 by 1

Nice 👍

I know this is a bit of a fiddly thing to write a test for, but if we're going to be relying on this logic in a number of place it would be good to have it under test

The service tests include a target (the GitLab repo itself) which has more than 100 releases so it is observable (albeit manually via TRACE_SERVICES). Probably some class level unit testing is possible with sufficient mocks, but feel like that's probably best investigated/implemented in a separate PR, thoughts?

Sounds like a plan 👍

calebcartwright · 2021-09-16T01:05:07Z

services/gitlab/gitlab-release.service.js

+  gitlab_url: optionalUrl,
+  include_prereleases: Joi.equal(''),
+  sort: Joi.string().valid('date', 'semver').default('date'),
+  display_name: Joi.string().valid('tag', 'release').default('tag'),


Figured it made sense to add this from day 0 given where we ended up with this on GitHub. I've opted to set the default to tag to be consistent with what GitHub currently is but don't have strong opinions on this one way or another.

chris48s · 2021-10-13T20:21:53Z

services/gitlab/gitlab-release.service.js

+
+  static route = {
+    base: 'gitlab/v/release',
+    pattern: ':user/:repo',


I probably should have picked this up in the GitLab tag PR review, but on Gitlab repos can be nested in subgroups e.g: https://gitlab.com/megabyte-labs/dockerfile/ci-pipeline/ansible-lint (refs #6427 ) . As we start adding a suite of GitLab badges, lets establish the convention that for new API based badges we work with this pattern and don't restirct to only the user/repo format (I think this means allowing an arbitrary number of slashes in this part of the URL). I think this also then implies that any new GitLab badges which take a branch param accept it as a query param ?branch= rather than a URL param (although this possibly depends a bit on the upstream API routes).

Two additional wrinkles to consider:

the API will also happily accept the Project Id (25813592 for ansible-lint), the value of which is displayed directly in the GitLab UI

when using the project path, it has to be url encoded (e.g. https://gitlab.com/api/v4/projects/megabyte-labs%2Fdockerfile%2Fci-pipeline%2Fansible-lint/releases)

I almost feel like this is going to force us to have a single param where users can either go the easy route and plugin the project id, or the full project path and we'll handle the uri encoding.

Yeah so I'm suggesting the pattern we adopt is /:repo+ for the [group(s)]/user/repo and then we pass encodeURIComponent(repo) to the API. Then for any badges where a branch param is valid, we implement that as ?branch=name. I think this pattern of nesting in groups is quite common for gitlab repos and I reckon it is worth allowing the 'friendly' form of the name rather than allowing you to use the repo name for some repositories but requiring the ID for others.

Yup, agreed. Believe bfe7c1c should have this all sorted now, though will need to circle back to how we best apply the pattern to other badges

chris48s · 2021-10-13T20:22:30Z

services/gitlab/gitlab-release.service.js

+    // https://docs.gitlab.com/ee/api/releases/
+    return this.fetchPaginatedArrayData({
+      schema,
+      url: `${baseUrl}/api/v4/projects/${user}%2F${repo}/releases`,


https://docs.gitlab.com/ee/api/releases/ says the default ordering is released_at so I guess we are saying ?sort=date corresponds to released_at (as opposed to created_at). Do you have a handle on the difference/tradeoff here? I wonder if it would be best to directly expose both of those options..

Do you have a handle on the difference/tradeoff here?

As far as the distinction between the two, my understanding is that it's analogous to GitHub's created_at and published_at fields respectively. The formers are the system derived values set to the true timestamp when the artifact was created, whereas the latters are a customizable field users can leverage to denote a "future" release, which I believe is only available when creating release via the API (for both Lab and Hub)

https://docs.gitlab.com/ee/api/releases/ says the default ordering is released_at so I guess we are saying ?sort=date corresponds to released_at (as opposed to created_at).

I could've sworn I'd left a note on this somewhere but can't find anything anywhere so 🤷‍♂️

In short, I've already found cases where their docs are just wrong as it relates to default ordering (see the Tags endpoint for example), so I don't personally put much stock into their claimed defaults.

The only case where I could potentially see this being relevant would be a scenario where someone is manually creating releases via the API and interleaving dates. That's probably a bit of an edge case, and one I'd prefer to wait to solve for only if someone actually asks, in part because we don't support the same for GitHub and because it introduces scenarios like if/how to account for future released_at/published_at dates.

We'd still have the flexibility down the road to add a new foo query param that could be used to toggle

OK. If we reckon the default in the docs may be misleading, should we explicitly pass an order_by param so we're clear which decision we're making them.
Trying to think about it from the point of view of compatibility with the GH badge, I'm now not actually sure what assumption the releases/latest endpoint makes on this.. I guess the reason I'm thinking we might expose both is because if we pick one then inevitably its the wrong one for someone. I'm thinking #6309 , #6236 , etc.

Good point, we should at a minimum be explicit both in the params we surface on our routes and in what we pass upstream to make us as "future proof" as possible to allow for any changes/enhancements down the line without risking breaking changes.

Believe cc62b85 should cover this, though still want to punt on any considerations of dealing with future date values with the released_at ordering to our future selves 😆

chris48s · 2021-10-13T20:23:00Z

services/gitlab/gitlab-base.js

+  async fetchPaginatedArrayData({
+    url,
+    options,
+    schema,
+    errorMessages,
+    firstPageOnly = false,
+  }) {
+    const requestParams = this.authHelper.withBasicAuth({
+      url,
+      options: {
+        headers: { Accept: 'application/json' },
+        qs: { per_page: 100 },
+        ...options,
+      },
+      errorMessages,
+    })
+
+    const {
+      res: { headers },
+      data,
+    } = await this.fetchPage({ page: 1, requestParams, schema })
+    const numberOfPages = headers['x-total-pages']
+
+    if (numberOfPages === 1 || firstPageOnly) {
+      return data
+    }
+
+    const pageData = await Promise.all(
+      [...Array(numberOfPages - 1).keys()].map((_, i) =>
+        this.fetchPage({ page: ++i + 1, requestParams, schema })
+      )
+    )
+    return [...data].concat(...pageData)
+  }


Nice 👍

I know this is a bit of a fiddly thing to write a test for, but if we're going to be relying on this logic in a number of place it would be good to have it under test

chris48s · 2021-10-16T18:55:02Z

services/gitlab/gitlab-release.tester.js

+t.create('Release (project id latest by date)')
+  .get('/29538796.json')
+  .expectBadge({ label: 'release', message: 'v2.0.0', color: 'blue' })
+


Here's an example of a gitlab project which is nested in a group and has releases we could use for a service test: https://gitlab.com/gitlab-org/frontend/eslint-plugin/-/releases

Awesome! Excellent find and/or memory 😆

calebcartwright · 2021-10-17T19:28:49Z

Some change on master has caused the tests to start failing, and I presume this is related to the switch from got to node-fetch.

Will try to figure it out and work around it, but wanted to make a note so we'll be sure to test if we flip back from node-fetch to something else

calebcartwright · 2021-10-17T20:12:06Z

Blocked on a decision/resolution of #7167

feat: add GitLabRelease badge

84a52af

calebcartwright added the service-badge New or updated service badge label Sep 15, 2021

shields-cd temporarily deployed to shields-staging-pr-7021 September 15, 2021 01:38 Inactive

calebcartwright mentioned this pull request Sep 15, 2021

Badge Request: GitLab tags and releases #4867

Closed

calebcartwright commented Sep 16, 2021

View reviewed changes

shields-cd temporarily deployed to shields-staging-pr-7021 September 25, 2021 03:01 Inactive

chris48s reviewed Oct 13, 2021

View reviewed changes

calebcartwright added 2 commits October 16, 2021 10:13

use single project route param

bfe7c1c

add query param for date ordering

cc62b85

calebcartwright mentioned this pull request Oct 16, 2021

Support nested groups on [GitLabTag] badge #7158

Merged

shields-cd temporarily deployed to shields-staging-pr-7021 October 16, 2021 18:24 Inactive

chris48s reviewed Oct 16, 2021

View reviewed changes

add test for nested subgroup

c3be565

calebcartwright temporarily deployed to shields-staging-pr-7021 October 16, 2021 19:10 Inactive

chris48s approved these changes Oct 17, 2021

View reviewed changes

calebcartwright added the squash when passing label Oct 17, 2021

Merge branch 'master' into gitlab-release

92821f5

shields-cd temporarily deployed to shields-staging-pr-7021 October 17, 2021 16:45 Inactive

Merge branch 'master' into gitlab-release

1a1246c

shields-cd temporarily deployed to shields-staging-pr-7021 October 17, 2021 17:02 Inactive

Merge branch 'master' into gitlab-release

318615d

shields-cd deployed to shields-staging-pr-7021 October 17, 2021 17:54 View deployment

calebcartwright mentioned this pull request Oct 17, 2021

node-fetch response headers "missing" due to API difference #7167

Closed

calebcartwright removed the squash when passing label Oct 17, 2021

Merge branch 'master' into gitlab-release

f32a86e

calebcartwright merged commit 50c0be0 into master Oct 23, 2021

calebcartwright deleted the gitlab-release branch October 23, 2021 03:08

chris48s mentioned this pull request Jan 10, 2022

Cannot Add Subgroup Repo for GitLab Code Coverage #7473

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add GitLab Release badge, run all [GitLab] #7021

add GitLab Release badge, run all [GitLab] #7021

calebcartwright commented Sep 15, 2021

shields-ci commented Sep 15, 2021 •

edited

Loading

calebcartwright Sep 16, 2021

chris48s Oct 13, 2021

calebcartwright Oct 13, 2021

chris48s Oct 14, 2021

calebcartwright Sep 16, 2021

chris48s Oct 13, 2021

calebcartwright Oct 13, 2021

chris48s Oct 14, 2021

calebcartwright Oct 16, 2021

chris48s Oct 13, 2021

calebcartwright Oct 13, 2021

chris48s Oct 14, 2021

calebcartwright Oct 16, 2021

chris48s Oct 13, 2021

chris48s Oct 16, 2021

calebcartwright Oct 16, 2021

calebcartwright commented Oct 17, 2021

calebcartwright commented Oct 17, 2021

add GitLab Release badge, run all [GitLab] #7021

add GitLab Release badge, run all [GitLab] #7021

Conversation

calebcartwright commented Sep 15, 2021

shields-ci commented Sep 15, 2021 • 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

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

calebcartwright commented Oct 17, 2021

calebcartwright commented Oct 17, 2021

shields-ci commented Sep 15, 2021 •

edited

Loading