refactor [circleci] integration #1927

chris48s · 2018-08-16T20:05:26Z

shields-ci · 2018-08-16T20:06:43Z

	Messages
📖	✨ Thanks for your contribution to Shields, @chris48s!
📖	Thanks for contributing to our documentation. We ❤️ our documentarians!

Generated by 🚫 dangerJS

chris48s · 2018-08-16T20:07:22Z

services/circleci/circleci.service.js

+
+const circleSchema = Joi.array()
+  .items(Joi.object({ status: Joi.string() }))
+  .min(1)


I think this is a sensible rule. I can't see a case for treating [] as a valid response here..

chris48s · 2018-08-16T20:07:46Z

services/circleci/circleci.service.js

+    return { message: status, color: color }
+  }
+
+  transform(data) {


Is this supposed to be a static method? I can't remember what the rule is now. When stuff should/shouldn't be static is definitely a topic we need to cover when we write docs.

Yea, in general you can make anything static that doesn't use this. The exception is if you're overriding superclass method, in which case making it static would break the overriding.

chris48s · 2018-08-16T20:08:53Z

services/circleci/circleci.service.js

+  async handle({ token, type, userRepo, branch }) {
+    const json = await this.fetch({ token, type, userRepo, branch })
+    const { status, color } = this.transform(json)
+    return this.constructor.render({ status, color })


As I write and review more code using this fetch()/render() pattern, I think we're really close to a point where we should update the docs to formalise this pattern. I think if #1922 (another complex service 'family') goes in without any major change to the base classes (beyond adding the Deprecated exception), we should make docs the next priority.

paulmelnikow · 2018-08-16T20:21:58Z

services/circleci/circleci.service.js

+const BaseJsonService = require('../base-json')
+
+const circleSchema = Joi.array()
+  .items(Joi.object({ status: Joi.string() }))


Should this be status: Joi.string().required()?

paulmelnikow · 2018-08-16T20:28:11Z

services/circleci/circleci.service.js

+  }
+
+  static render({ status, color }) {
+    return { message: status, color: color }


We should turn on the eslint rule for this; color: color can just be color.

paulmelnikow · 2018-08-16T20:29:23Z

services/circleci/circleci.service.js

+    return { message: status, color: color }
+  }
+
+  transform(data) {


Yea, in general you can make anything static that doesn't use this. The exception is if you're overriding superclass method, in which case making it static would break the overriding.

paulmelnikow · 2018-08-16T20:38:11Z

services/circleci/circleci.service.js

+      } else {
+        color = 'lightgrey'
+        shieldsStatus = circleStatus.replace('_', ' ')
+        return { status: shieldsStatus, color: color }


So it seems the purpose of this function is to summarize the multiple statuses down to a single status. I'm fine with that, though it does seem possible that it's better thought of as a rendering concern. For example, we could have a badge option that showed these like 2 passed, 1 failed.

Also, this looks buggy. What if the first test is scheduled and the second is failed? That should be reported as failed, but I think it will show as scheduled instead. Probably it would be better to check if any of the statuses are 'failed', then move down the list.

Would it be cleaner (and less buggy) to return an array of statuses, or extract an array of statuses in handle(), and delegate the summarizing and coloring to render()?

This is a very good point which I did not consider when I was looking at #1815 but now I think about this, there are many worms in this can..

It is clear that if all tests are passing, that should summarise to 'pass', and if any of the tests are failing, that should summarise to 'fail', but if we get back something like:

[ { 'status': 'running' }, { 'status': 'infrastructure_fail' }, { 'status': 'scheduled' }, ]

or

[ { 'status': 'success' }, { 'status': 'success' }, { 'status': 'queued' }, { 'status': 'no_tests' }, ]

what is an appropriate summary of that?

Looking at the docs, it seems that we could get any arbitrary combination of:

retried

canceled

infrastructure_fail

timedout

not_run

running

failed

queued

scheduled

not_running

no_tests

fixed

success

[ { 'status': 'running' }, { 'status': 'infrastructure_fail' }, { 'status': 'scheduled' }, ]

-> infrastructure_fail

[ { 'status': 'success' }, { 'status': 'success' }, { 'status': 'queued' }, { 'status': 'no_tests' }, ]

Huh, is that actually possible? no tests seems to mean there would be no other results. If that ever happened I'd be okay displaying queued or indeterminate.

In the case of

[ { 'status': 'success' }, { 'status': 'success' }, { 'status': 'no_tests' }, ]

I think indeterminate is appropriate.

Maybe we could prioritize the results by severity, and display the highest severity result that appears? We'll have to make some judgement calls when we do that, though I think that's okay.

We could special-case no tests which really should only exist on its own.

I think you might have taken those examples too literally - I just made up some arbitrary combinations :)

However, its very useful that this has prompted me to think about this more clearly and properly read the documentation because it turns out:

a) I've been trying to solve completely the wrong problem
b) It is only through luck that #1815 has not actually broken anything for users

What I thought I've been doing here is combining the statuses of a list of jobs/workflows to make a build status, but what I've actually been trying to do is combine >1 build statuses (which already summarise multipe jobs/workflows) to make.. a thing that makes no sense. By sheer good luck, we were passing &limit=1 the whole time so in the real world it all made no difference anyway because we've only ever really been operating on an array of length 1 (despite having written some tests which assume an array of length >1).

So.. the upshot of all that is:

Fix [circleci] badge #1815 was completely pointless and fixed nothing - I've put the code back how it was before I started in 90160bc

I've also added validation and a test to clarify that we are expecting an array of length 1 and removed the pointless tests in 90160bc

Whatever the issue was in Our test badge incorrectly stays green #1792 is not fixed and we should re-open that issue. I was completely wrong about the problem.

🤦‍♂️

I agree with much of that, though I was just looking at https://circleci.com/docs/api/v1-reference/ and it seems to me that a build does not combine multiple jobs. A build seems to refer to e.g. a single tile on https://circleci.com/gh/badges/shields = an individual job.

https://circleci.com/api/v1.1/project/github/badges/shields

Unless outcome and status are conveying different things, or there's something else I'm not seeing…

I feel like outcome may actually be the one we are looking for:

"outcome" : "failed", // :canceled, :infrastructure_fail, :timedout, :failed, :no_tests or :success "status" : "failed", // :retried, :canceled, :infrastructure_fail, :timedout, :not_run, :running, :failed, :queued, :scheduled, :not_running, :no_tests, :fixed, :success

as those possible values seem to be more related to the overall build in general

Right, yep so we are only looking at one job when there may be N jobs in the build (although I think we are using using the wrong terminology there as the docs refer to one record in this array as a 'build'). I'm going to refer to 'build' and 'group' moving forwards.

status and outcome seem to both relate to a build. For example, if we look at the branch dependabot/npm_and_yarn/simple-icons-1.8.4 build_nums 11388, 11387, 11386, 11385 and 11384 all group together, but if we look at the status and outcome values:

build_num status outcome 11388 fixed success 11387 failed failed 11386 success success 11385 failed failed 11384 failed failed

Although 'outcome' is probably useful to reduce the number of possible values we have to deal with, if it was summarising the group, I'd expect them all to be 'failed'.

So we do need to aggregate multiple status (or outcome values). What I'm slightly struggling with now is how to connect builds in an array like this (which contains builds relating to multiple groups) as being part of the same group (which is what we're trying to summarise).

One thing I have thought of is we have the vcs_revision property, so we can identify builds in the array which were run against the same commit (on the same branch). Is that a solid way of doing this, or can we run the same build more than once on the same commit? I think if we restart a build it re-uses the same number.

Looking at an example, picking all of the objects with the same vcs_revision is going to double-count retried builds. For example:
https://circleci.com/gh/badges/shields/11764 and
https://circleci.com/gh/badges/shields/11769
are both services-pr on the same commit hash on the same branch.

Can we use workflows.workflow_id?

workflows: { job_name: "frontend", job_id: "ef27df67-7aad-4022-a159-2c55718da667", workflow_id: "5b94f353-2060-4c80-8b35-941174632ce3", workspace_id: "5b94f353-2060-4c80-8b35-941174632ce3", upstream_job_ids: [ "4fac3028-db01-4497-a3c5-97c75194eb0d" ], upstream_concurrency_map: { }, workflow_name: "on-commit" }

Yeah, i think that is going to be the only way, although we are also going to need to increase/remove the ?limit=

paulmelnikow · 2018-08-16T20:39:49Z

services/circleci/circleci.service.js

+      base: 'circleci',
+      format:
+        '(?:token/(w+))?[+/]?project/(?:(github|bitbucket)/)?([^/]+/[^/]+)(?:/(.*))?',
+      capture: ['token', 'type', 'userRepo', 'branch'],


Re type: Circle calls this vcsType which seems a bit clearer.

RedSparr0w

Nice work, Looks good.
👍

RedSparr0w · 2018-08-19T21:32:23Z

services/circleci/circleci.service.js

+
+  // Metadata
+  static get defaultBadgeData() {
+    return { label: 'build' }


Do you think it would be worth leaving this out as it defaults to the category already?

Having chewed this over, I think it is useful to keep labels explicit.

It is helpful that we can fall back to category to ensure we do have a label, but if you think about the re-organisation work going on in #1905 (comment) I wouldn't want to be in a situation where we move badges around and all the labels change.

For example, if we renamed the 'Build' category to 'Continuous Integration' (not saying we should do this - just an example), we would probably still want this label to be 'build'. Lets not create a situation where doing something like that creates a domino effect causing unpredictable changes elsewhere.

paulmelnikow · 2018-08-27T17:38:21Z

Hey @chris48s this needs some conflict resolution. Since the changes here to server.js are simple, it should be easiest to grab the version from master, and then delete the circle implementation (and remove unused helper functions). I'm planning to validate the master branch and deploy tonight (UTC-7). It would be great to include this one, too!

chris48s · 2018-08-27T18:17:26Z

It would be great to include this one, too!

There are 2 bits of work going on in this PR:

Refactoring the existing code (which has a bug)
Fixing a bug

Do you mean you want to get the refactored (but still buggy) code into a state where you can merge it and then treat the bugfix as a different job?

If you want to just get all the service code out of server.js another option is to convert this into a LegacyService and I'll just rebase and deal with it.. I don't think it makes much difference.

paulmelnikow · 2018-08-27T18:28:54Z

Ah, no, sorry, I forgot about the issue with multiple builds. I’m flexible. We can do this in whatever order makes the most sense to you.

- merge in master - use :token placeholder syntax - convert live examples to static examples

chris48s · 2018-08-27T19:32:40Z

Right, I've sorted out the merge conflicts and pushed that. What's here now is refactored into the new structure maintaining the existing functionality (only looking at the most recent one build).

I've had a look at some examples and I think using workflows.workflow_id to group related builds is the right thing to do so I'm going to look at that next.

If you want to merge this in the meantime, I can submit that as a new PR. If not, I'll append it to this one.

…into circle-refactor

chris48s · 2018-08-28T14:13:58Z

I spotted another issue with the way I've done the static examples, so this now needs another review.

I'm now quite close to being able to submit a fix for #1792 but this has spiralled into quite a complex job as I've found more and more edge cases. The fix will require adding a lot of additional code and tests. As such, I think once I've got another 👍 on this I'll merge this as it stands and we will deal with that as another PR to make the review easier to deal with because we won't be trying to deal with that and the refactoring all at once.

shields-deployment · 2018-08-28T14:49:46Z

This pull request was merged to master branch. Now this change is waiting for deployment.
Deploys usually happen every few weeks. After deployment changes are copied to gh-pages branch.

This badge displays deployment status:

chris48s added 2 commits August 16, 2018 20:49

refactor [circleci] integration

1ebc754

remove unused import

2f0fa66

chris48s commented Aug 16, 2018

View reviewed changes

chris48s added the service-badge New or updated service badge label Aug 16, 2018

paulmelnikow mentioned this pull request Aug 16, 2018

The Great Service Rewrite 🏆 #1358

Closed

paulmelnikow reviewed Aug 16, 2018

View reviewed changes

address some review comments

1efec96

chris48s force-pushed the circle-refactor branch from 0e40f4c to 1efec96 Compare August 17, 2018 18:43

only expect one build status

90160bc

paulmelnikow mentioned this pull request Aug 19, 2018

Our test badge incorrectly stays green #1792

Closed

RedSparr0w previously approved these changes Aug 19, 2018

View reviewed changes

chris48s added 2 commits August 27, 2018 20:08

update examples (merge branch 'master')

9e1899c

- merge in master - use :token placeholder syntax - convert live examples to static examples

Merge branch 'master' into circle-refactor

1392839

chris48s dismissed RedSparr0w’s stale review via 1392839 August 27, 2018 19:22

paulmelnikow previously approved these changes Aug 27, 2018

View reviewed changes

paulmelnikow and others added 3 commits August 27, 2018 16:38

Merge branch 'master' into circle-refactor

562923d

fix static examples

64974cb

Merge branch 'circle-refactor' of https://github.com/chris48s/shields …

42e6c0e

…into circle-refactor

chris48s dismissed paulmelnikow’s stale review via 42e6c0e August 28, 2018 14:06

chris48s mentioned this pull request Aug 28, 2018

Fix wercker examples, make static examples less error prone #1994

Closed

paulmelnikow approved these changes Aug 28, 2018

View reviewed changes

Merge branch 'master' into circle-refactor

30777f7

chris48s merged commit e294dc4 into badges:master Aug 28, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor [circleci] integration #1927

refactor [circleci] integration #1927

chris48s commented Aug 16, 2018

shields-ci commented Aug 16, 2018

chris48s Aug 16, 2018

chris48s Aug 16, 2018

paulmelnikow Aug 16, 2018

chris48s Aug 16, 2018

paulmelnikow Aug 16, 2018

paulmelnikow Aug 16, 2018

paulmelnikow Aug 16, 2018

paulmelnikow Aug 16, 2018

chris48s Aug 17, 2018 •

edited

Loading

paulmelnikow Aug 17, 2018

chris48s Aug 18, 2018

paulmelnikow Aug 18, 2018

RedSparr0w Aug 19, 2018 •

edited

Loading

chris48s Aug 20, 2018

chris48s Aug 23, 2018

paulmelnikow Aug 23, 2018

RedSparr0w Aug 23, 2018 •

edited

Loading

paulmelnikow Aug 16, 2018

RedSparr0w left a comment

RedSparr0w Aug 19, 2018

chris48s Aug 27, 2018

paulmelnikow commented Aug 27, 2018

chris48s commented Aug 27, 2018

paulmelnikow commented Aug 27, 2018

chris48s commented Aug 27, 2018

chris48s commented Aug 28, 2018

shields-deployment bot commented Aug 28, 2018

refactor [circleci] integration #1927

refactor [circleci] integration #1927

Conversation

chris48s commented Aug 16, 2018

shields-ci commented Aug 16, 2018

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

chris48s Aug 17, 2018 • 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

RedSparr0w Aug 19, 2018 • 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

RedSparr0w Aug 23, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RedSparr0w left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paulmelnikow commented Aug 27, 2018

chris48s commented Aug 27, 2018

paulmelnikow commented Aug 27, 2018

chris48s commented Aug 27, 2018

chris48s commented Aug 28, 2018

shields-deployment bot commented Aug 28, 2018

chris48s Aug 17, 2018 •

edited

Loading

RedSparr0w Aug 19, 2018 •

edited

Loading

RedSparr0w Aug 23, 2018 •

edited

Loading