fix [circleci] badge for projects that use workflows #1995

chris48s · 2018-08-28T15:40:49Z

Well this escalated in complexity quite sharply...

In an attempt to solve #1792 I've had a go at grouping related builds based on workflow_id. There are a number of changes in this PR which are useful to discuss 'inline', so I will mark up the diff with specific line comments, but just to give a high-level summary this is how the new approach works:

Attempt to work out if this project uses workflows
If it doesn't use workflows, just grab the outcome of the latest build and return that
If it does use workflows,
- Find the most recent workflow where all the builds with that workflow_id have finished
- Grab all the builds with that workflow_id
- Summarise all the build outcomes into a single outcome for the workflow

In general, this seems to work pretty well. Unfortunately there are 3 edge cases (that I can think of) which could be problematic:

If a workflow with multiple builds is currently in progress and some builds have finished and some have not yet been started/queued, we might summarise an incomplete build. I'm not sure exactly how likely it is that we would hit this as testing it requires some quite precise timing but even if this is possible I don't think there is any way we could fix or work around it using this API.
We've hard-coded a limit on the number of builds, but if a project has a large build matrix there could potentially be a situation (e.g: if multiple workflows currently have builds in progress and we have to summarise the third or fourth workflow back in time), we might end up summarising only part of a build because some of the builds in the wrokflow are beyond our hard-coded limit. If this did happen it would fail by producing an incorrect/unexpected value and be very hard to detect. Again, I see no sensible way to fix or work around this using this API.
If a project has a mix of recent builds that do use workflows and older builds that don't use workflows in its history (I actually have a good example of this: https://circleci.com/api/v1.1/project/github/DemocracyClub/ec2-tag-conditional/tree/master ), we will fail with an InvalidResponse() until every build in the build history specified by the limit param either does or doesn't use workflows. This case fails with an error rather than unexpected output (so is less serious). Also it may be possible to account for, but I've not attempted to do so.

shields-ci · 2018-08-28T15:42:18Z

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

Generated by 🚫 dangerJS

chris48s · 2018-08-28T15:50:28Z

services/circleci/circleci.helpers.spec.js

+})
+
+describe('circleci: schema validation', function() {
+  const validate = function(data, schema) {


This may not be the best place to get bogged down in it, but it seems like writing tests for non-trivial validators (like this one) is something we might want to do sometimes. I definitely found I needed to write tests to get this one correct. Lets have a think about a good pattern for this.

chris48s · 2018-08-28T15:50:57Z

services/circleci/circleci.helpers.js

+const emptyArraySchema = Joi.array()
+  .min(0)
+  .max(0)
+  .required() // [] is also a valid response from Circle CI


Turns out sometimes [] is a legit response example: https://circleci.com/api/v1.1/project/github/chris48s/shields/tree/issue1827

chris48s · 2018-08-28T15:51:38Z

services/circleci/circleci.service.js


 module.exports = class CircleCi extends BaseJsonService {
  async fetch({ token, vcsType, userRepo, branch }) {
    let url = `https://circleci.com/api/v1.1/project/${vcsType}/${userRepo}`
    if (branch != null) {
      url += `/tree/${branch}`
    }
-    const query = { filter: 'completed', limit: 1 }
+    const query = { limit: 50 }


We can't filter on 'completed' any more here. We need to know about in-progress/queued builds so that if the first workflow we come across has some builds which are not complete we know to ignore that one and move on to the next workflow to try and find one where all the builds are complete. If we filter on complete here, sometimes we will summarise an incomplete workflow.

In terms of limit, I've picked 50 to balance 2 properties:

We need a fairly large number of records so that on a project with a large build matrix where we can't take the first workflow_id (e.g: because some builds are still in progress) we've got enough records

If we fetch loads of records the badge takes a long time to render because we must wait a long time for the fetch() stage

chris48s · 2018-08-28T15:52:22Z

services/circleci/circleci.helpers.js

+  } else if (counts.timedout >= 1) {
+    return 'timed out'
+  } else if (counts.failed >= 1) {
+    return 'failed'


I've taken a stab at a sensible order here, but I'm not committed to it. If you're reading this thinking "'canceled' obviously takes precedence over 'infrastructure fail'", feel free to make suggestions and I'll change it.

I've also switched to using 'outcome' here instead of status (outcome only applies to finished builds and we only want to summarise workflows where all the builds are finished). This also gives as smaller number of values to try and make sense of.

chris48s · 2018-08-28T15:53:45Z

services/circleci/circleci.service.js

+    ) {
+      color = 'red'
+    } else if (status === 'no tests') {
+      color = 'yellow'


Same here. I've attempted to do something sensible-ish, but again if you're thinking "'canceled' should obviously be grey" or whatever, feel free to make suggestions and I'll change them. Once we've got this nailed I'll also add tests for the colour logic. There is relatively little consistency about this on other services. Prior art to maybe consider:

https://github.com/amio/badgen-service/blob/master/libs/live-fns/circleci.js

https://github.com/circleci/frontend/blob/c189f3546afe49b64c8ee86d92ff67ed9d2eda78/src-cljs/frontend/models/build.cljs#L174-L181
(although they are both looking at 'status' as opposed to 'outcome')

chris48s · 2018-08-28T15:54:09Z

services/circleci/circleci.tester.js

-    value: 'invalid json response',
-    colorB: '#9f9f9f',
-  })
+  .expectJSON({ name: 'build', value: 'could not summarize build status' })


I've implemented a lot of the special-case testing for this badge logic in circleci.helpers.spec.js instead of using a lot of mocked responses here. I've kept the service tests pretty high-level

- use lodash.countby in countOutcomes() - use lodash.countby in countOutcomes() - move validation from countOutcomes() into schema - update tests - switch getLatestCompleteBuildOutcome() to use filter()

resolve conflict: keep ours

chris48s · 2018-09-09T19:18:17Z

Based on chat in discord, I've updated this PR to use a more functional style in a couple more places in b6cda72 .

I think I'm going to leave getBuildsForLatestCompleteWorkflow() where it is. I find the mutable state approach easier to read here than the proposed uniq(builds.map(getWorkflowId)).find(workflowId => builds.filter(…).every(…))) alternative, although 'readable' is a matter of taste :)

Using lodash.countby in countOutcomes() is probably the big readabiliy gain, particularly as refactoring that function has also prompted me to move more validation logic into the schema - which is the correct place for it 👍

resolve conflicts

chris48s · 2018-11-16T20:27:38Z

I've resolved the conflicts on this if you want to pick up the review. I've slightly forgotten what was going on here but usefully I seem to have left a bunch of notes on the diff so hopefully it shouldn't be too hard to resume. Well done me from the past :)

paulmelnikow · 2019-01-08T18:28:11Z

It sounds like we're thinking #1064 is a better way to go because it will let us rely on Circle's own logic.

fix [circleci] badge for projects that use workflows

28fa27e

coding standards

55b0d27

chris48s force-pushed the issue1792-part2 branch from 768d2b5 to 28fa27e Compare August 28, 2018 15:44

chris48s commented Aug 28, 2018

View reviewed changes

chris48s added 3 commits September 9, 2018 19:59

use more functional style

b6cda72

- use lodash.countby in countOutcomes() - use lodash.countby in countOutcomes() - move validation from countOutcomes() into schema - update tests - switch getLatestCompleteBuildOutcome() to use filter()

Merge branch 'master' into issue1792-part2

feb56f5

resolve conflict: keep ours

update coding standards

2e28cbb

chris48s added 3 commits September 18, 2018 19:57

Merge branch 'master' into issue1792-part2

4e7cc34

resolve conflicts

fix token example

14522fe

coding standards

5757a37

chris48s added the service-badge New or updated service badge label Sep 25, 2018

paulmelnikow added the bug Bugs in badges and the frontend label Nov 15, 2018

Merge and resolve conflicts

205344a

chris48s added 2 commits November 18, 2018 13:59

Merge branch 'master' into issue1792-part2

b2dc7f2

urlPattern --> pattern

75d5dce

paulmelnikow mentioned this pull request Nov 21, 2018

CircleCI : use only Status permission for the API calls #1064

Closed

dependabot-preview bot mentioned this pull request Dec 31, 2018

Bump eslint-plugin-react from 7.11.1 to 7.12.0 #2609

Merged

paulmelnikow closed this Jan 8, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix [circleci] badge for projects that use workflows #1995

fix [circleci] badge for projects that use workflows #1995

chris48s commented Aug 28, 2018

shields-ci commented Aug 28, 2018 •

edited

Loading

chris48s Aug 28, 2018

chris48s Aug 28, 2018

chris48s Aug 28, 2018

chris48s Aug 28, 2018

chris48s Aug 28, 2018

chris48s Aug 28, 2018

chris48s commented Sep 9, 2018

chris48s commented Nov 16, 2018

paulmelnikow commented Jan 8, 2019

fix [circleci] badge for projects that use workflows #1995

fix [circleci] badge for projects that use workflows #1995

Conversation

chris48s commented Aug 28, 2018

shields-ci commented Aug 28, 2018 • edited Loading

chris48s Aug 28, 2018

Choose a reason for hiding this comment

chris48s Aug 28, 2018

Choose a reason for hiding this comment

chris48s Aug 28, 2018

Choose a reason for hiding this comment

chris48s Aug 28, 2018

Choose a reason for hiding this comment

chris48s Aug 28, 2018

Choose a reason for hiding this comment

chris48s Aug 28, 2018

Choose a reason for hiding this comment

chris48s commented Sep 9, 2018

chris48s commented Nov 16, 2018

paulmelnikow commented Jan 8, 2019

shields-ci commented Aug 28, 2018 •

edited

Loading