[MicroBadger] Add MicroBadger #1340

goozler · 2017-12-06T11:28:10Z

Added support for MicroBadger image size and layers with ability to choose a tag (#944)

goozler · 2017-12-06T11:44:19Z

I've seen the pull request with CircleCI and there "flaky tests" are mentioned. To be honest, locally my new tests fail sometimes due to to a long response from the MicroBadger API (more than 2 seconds).

I don't know what the best solution in that case. I can stub every request but I saw in other PRs that you ask to add a few "live" tests, so, I need your suggestion here.

paulmelnikow · 2017-12-06T16:17:10Z

Hi, thanks for this!

I've seen the pull request with CircleCI and there "flaky tests" are mentioned.

I should clarify that. We want tests that hit the real services. They're unreliable by necessity. I should think of a value-neutral way to say "unreliable." 😜 There's a long discussion of the test strategy in #927.

To be honest, locally my new tests fail sometimes due to to a long response from the MicroBadger API (more than 2 seconds).

Since readme images are served through GitHub's proxy (camo) which has its own timeout, they may be unreliable there too.

We will increase the test timeout, so tests that occasionally take longer than 2s do not cause failures. This is waiting on a release of IcedFrisby which includes IcedFrisby/IcedFrisby#71 which I should wrap up!

goozler · 2017-12-06T17:09:18Z

Ok, that makes sense! So, let me know if you need something more from my side to merge it. Feel free to make notes about the code.

paulmelnikow · 2017-12-06T19:04:32Z

Would anyone care to review this? @PyvesB @platan @chris48s

PyvesB · 2017-12-06T19:16:26Z

@paulmelnikow : sure, I'll take care of it later in the evening. 😉

chris48s

Thanks very much for working on this. I've left some comments for things you could improve but its shaping up nicely :) Some of my comments may benefit from clarification from another reviewer.

chris48s · 2017-12-06T21:04:23Z

service-tests/microbadger.js

+  .get('/image-size/_/httpd/alpine.json')
+  .expectJSONTypes(Joi.object().keys({
+    name: 'image size',
+    value: Joi.string().regex(/^[0-9]+[kMGTPEZY]B$/)


I think the convention for this project is that when writing a service test for a badge that uses metric(), the test should make a call that should generate a non-zero value and check the result with a regex that ensures the badge value is non-zero: /^[1-9][0-9]*[kMGTPEZY]?$/. See https://github.com/badges/shields/blob/master/service-tests/helpers/validators.js#L38-L43

We also have isFileSize, though probably it should be modified to reject 0 as well!

chris48s · 2017-12-06T21:05:14Z

server.js

+      var image;
+
+      if (tag) {
+        if (!parsedData['Versions']) {


Does this project have a convention for dot-notation vs square-bracket on object properties, or we not care?

I see we waiting for the answer of another maintainer but personally, I use square-bracket only when the key has an uppercase first letter.

For historical reasons server.js has really lax rules, and we're about to rewrite the services which will slowly empty it out. Elsewhere our code uses dots on any possible key (c.f. eslint-config-standard).

chris48s · 2017-12-06T21:05:43Z

server.js

+    user = 'library';
+  }
+  var path = user + '/' + repo;
+  var url = 'https://api.microbadger.com/v1/images/' + path;


I know a lot of the code in server.js uses var, but when adding new code maybe it would be better to make use of let and const where possible. For example url, repo, format etc here are declared and never re-assigned so could be declared with const. Similarly, i, j, versionJson, tagJson further down are unnecessarily defined with function scope and could be block-scoped by declaring them with let.

Yep, I'll change it. Totally makes sense. I just see that the most part uses var and the linter doesn't complain, so, I tried to be like majority :)

chris48s · 2017-12-06T21:06:07Z

server.js

+        return;
+      }
+
+      if (type == 'image-size') {


Can we compare using === here

chris48s · 2017-12-06T21:06:14Z

server.js

+      if (type == 'image-size') {
+        var size = metric(image['DownloadSize']) + "B";
+        badgeData.text[1] = size;
+      } else if (type == 'layers') {


...and here

chris48s · 2017-12-06T21:07:26Z

server.js

+      } else {
+        image = parsedData;
+      }
+


There's a chunk of logic in the middle here which parses the api response to find the latest tag. Could this be extracted to a helper?

To follow convention, you would create /lib/microbadger-helpers.js and move this bit of logic to a function getLatestTag() or getLatestVersionTag() or something. That would reduce the amount of branching/logic in this function making it easier to read and by factoring it out it would be easier to test. You could add few unit tests for it in /lib/microbadger-spec.js.

If you have a look through /lib there are some examples following this pattern that you can crib from.

Sure, I'll take a look! Thank you for the tip. I've just seen several other services with similar complexity code but I would like to break off that code too.

chris48s · 2017-12-06T21:18:36Z

oh, also could you rename the pull request so it is prefixed with [MicroBadger] before pushing additional commits to your branch please. That will allow the CI build to identify that it should run the tests in service-tests/microbadger.js when it runs the build. Cheers

PyvesB · 2017-12-06T21:20:46Z

@chris48s did you not see my comment above? I had literally almost finished my review and, as one would expect, I had picked up mostly similar comments to yours (see below for some extra points). While consecutive reviews may be valuable, there is no point in several people reviewing the exact same code at the exact same time and stepping on each other's toes. I guess I have wasted my time and can discard most of my work.

chris48s · 2017-12-06T21:29:24Z

@PyvesB - oh no! Sorry. We must have been looking at it at the same time. Really sorry we've duplicated effort here. I'll make sure I communicate more clearly in future to avoid this :(

PyvesB · 2017-12-06T21:37:09Z

service-tests/microbadger.js

+t.create('layers without a specified tag')
+  .get('/layers/_/hello-world.json')
+  .expectJSONTypes(Joi.object().keys({ name: 'layers',
+    value: Joi.number().integer()


You could probably make this slightly more restrictive by using Joi.number().integer().positive(). Also, I believe the formatting is not entirely consistent with your other tests.

PyvesB · 2017-12-06T21:39:09Z

server.js

+      }
+
+      if (type == 'image-size') {
+        var size = metric(image['DownloadSize']) + "B";


Could you use our prettyBytes helper function here?

Thanks! I like it

PyvesB · 2017-12-06T21:39:35Z

server.js

+  if (user === '_') {
+    user = 'library';
+  }
+  var path = user + '/' + repo;


Would this be a good candidate for using template strings?

Yep, why not

paulmelnikow

Looks good! Thank you @platan @chris48s for your reviews!

paulmelnikow · 2017-12-06T21:51:23Z

lib/all-badge-examples.js

+        previewUri: '/microbadger/image-size/_/httpd.svg',
+        keywords: [
+          'docker',
+          'microbadger'


'microbadger' is not needed since it is in the title. Seems there are some others that should be cleaned up as well. I'd love to set up Danger to catch things like this!

paulmelnikow · 2017-12-06T21:51:42Z

lib/all-badge-examples.js

+      },
+      {
+        title: 'MicroBadger Layers',
+        previewUri: '/microbadger/layers/_/httpd.svg',


Does layers support tag as well? Perhaps it would be worth including a fourth example.

goozler · 2017-12-06T22:14:08Z

Thank everyone for the review. I tried to fix all of your notes except extracting the parsing logic into a helper. I'll make an effort to do it tomorrow

paulmelnikow · 2017-12-07T03:46:04Z

You can get CircleCI running again by merging master into this branch.

goozler · 2017-12-07T08:40:44Z

@paulmelnikow I've updated the pull request with adding a helper. I didn't add a test for it because the helper is tested already by the general tests and to be honest I'm too lazy to add something more to test the same thing. I would have liked to add it if had thought about it at the beginning of the development because it could be useful in that case.

paulmelnikow · 2017-12-07T21:25:34Z

lib/microbadger-helpers.js

+      }
+    }
+  }
+  return result;


If I'm understanding correctly, this would be equivalent:

return data.Versions.find(v => v.Tags.some(t => t.tag === tag))

Since you only call it once, you could move that code to the handler and eliminate the helper.

Refs:

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/find

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/some

Yes, you got it correctly. Thanks for the refs though!

goozler · 2017-12-07T21:42:31Z

@paulmelnikow I removed the helper a changed conditions a little

paulmelnikow · 2017-12-07T22:30:54Z

Thanks for your work!

paulmelnikow added the service-badge Accepted and actionable changes, features, and bugs label Dec 6, 2017

chris48s reviewed Dec 6, 2017

View reviewed changes

PyvesB reviewed Dec 6, 2017

View reviewed changes

paulmelnikow reviewed Dec 6, 2017

View reviewed changes

goozler changed the title ~~Add MicroBadger~~ [MicroBadger] Add MicroBadger Dec 6, 2017

goozler force-pushed the add_microbadger branch from b520f13 to b05381e Compare December 7, 2017 08:32

Add MicroBadger

0368487

goozler force-pushed the add_microbadger branch from b05381e to 0368487 Compare December 7, 2017 11:47

paulmelnikow reviewed Dec 7, 2017

View reviewed changes

Get rid of the helper

db0f0ba

paulmelnikow merged commit b1ca639 into badges:master Dec 7, 2017

goozler deleted the add_microbadger branch January 15, 2018 12:51

dependabot-preview bot mentioned this pull request May 27, 2019

Build(deps-dev): bump eslint-plugin-import from 2.17.2 to 2.17.3 #3489

Merged

[MicroBadger] Add MicroBadger #1340

[MicroBadger] Add MicroBadger #1340

Conversation

goozler commented Dec 6, 2017 • edited Loading

goozler commented Dec 6, 2017 • edited Loading

paulmelnikow commented Dec 6, 2017

goozler commented Dec 6, 2017

paulmelnikow commented Dec 6, 2017

PyvesB commented Dec 6, 2017 • edited Loading

chris48s 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

Choose a reason for hiding this comment

chris48s commented Dec 6, 2017

PyvesB commented Dec 6, 2017 • edited Loading

chris48s commented Dec 6, 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

paulmelnikow left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

goozler commented Dec 6, 2017

paulmelnikow commented Dec 7, 2017

goozler commented Dec 7, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

goozler commented Dec 7, 2017

paulmelnikow commented Dec 7, 2017

goozler commented Dec 6, 2017 •

edited

Loading

goozler commented Dec 6, 2017 •

edited

Loading

PyvesB commented Dec 6, 2017 •

edited

Loading

PyvesB commented Dec 6, 2017 •

edited

Loading

goozler commented Dec 7, 2017 •

edited

Loading