Add [dynamic-yaml] badge #1623

jacobtomlinson · 2018-03-29T15:57:51Z

This PR closes #1617 by adding support for a dynamic YAML badge.

I have added js-yaml to handle the YAML parsing, but then the rest of the work has mostly been duplicating the JSON dynamic badge as YAML is just a superset of JSON.

I've tested it locally and run the test suite with no issues showing in the tests I added.

Please let me know if there is anything you want me to change.

shields-ci · 2018-03-29T15:58:55Z

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

Generated by 🚫 dangerJS

paulmelnikow

Thank you, this looks great! Main thing is to update the tests so they're exercising the code using a yaml data source.

paulmelnikow · 2018-03-29T16:50:50Z

services/yaml/yaml.tester.js

+
+t.create('YAML from url')
+  .get('.json?url=https://github.com/badges/shields/raw/master/package.json&query=$.name&style=_shields_test')
+  .expectJSON({ name: 'custom badge', value: 'gh-badges', colorB: colorsB.brightgreen });


Wouldn't these tests need to pull from a yml file instead of a json file?

paulmelnikow · 2018-03-29T16:52:33Z

frontend/components/usage.js

@@ -123,6 +123,9 @@ export default class Usage extends React.PureComponent {
        <p>
          <code>/badge/dynamic/json.svg?url=&lt;URL&gt;&amp;label=&lt;LABEL&gt;&amp;query=&lt;<a href="https://www.npmjs.com/package/jsonpath" target="_BLANK" title="JSONdata syntax">$.DATA.SUBDATA</a>&gt;&amp;colorB=&lt;COLOR&gt;&amp;prefix=&lt;PREFIX&gt;&amp;suffix=&lt;SUFFIX&gt;</code>
        </p>
+        <p>
+          <code>/badge/dynamic/yaml.svg?url=&lt;URL&gt;&amp;label=&lt;LABEL&gt;&amp;query=&lt;<a href="https://www.npmjs.com/package/jsonpath" target="_BLANK" title="JSONdata syntax">$.DATA.SUBDATA</a>&gt;&amp;colorB=&lt;COLOR&gt;&amp;prefix=&lt;PREFIX&gt;&amp;suffix=&lt;SUFFIX&gt;</code>
+        </p>


Could you also add yaml to the list in the dynamic badge maker?

jacobtomlinson · 2018-03-29T17:40:01Z

I thought it was valid to leave the JSON tests as JSON is valid YAML. I can definitely add more which explicitly tests the YAML syntax.

jacobtomlinson · 2018-03-30T07:49:19Z

I have updated the tests to use a YAML data source and added yaml to the list of options in the dynamic badge maker as requested.

Please let me know if there is anything else you need me to change.

paulmelnikow · 2018-03-30T07:52:14Z

JSON is valid YAML?! Mind = blown 😀

Thanks for the updates. I'll give this another read, and perhaps someone else would like to as well.

platan · 2018-03-30T09:27:09Z

services/yaml/yaml.tester.js

+  .expectJSON({ name: 'Package Name', value: 'no url specified', colorB: colorsB.red });
+
+t.create('No query specified')
+  .get('.json?url=https://raw.githubusercontent.com/kubernetes/charts/568291d6e476c39ca8322c30c3f601d0383d4760/stable/coredns/Chart.yaml&label=Package Name&style=_shields_test')


You've used a link to specified commit, which great in my opinion :-)

👍 I thought it would be consistent when testing.

platan · 2018-03-30T09:30:44Z

services/yaml/yaml.tester.js

+module.exports = t;
+
+t.create('Connection error')
+  .get('.json?url=https://github.com/badges/shields/raw/master/package.json&query=$.name&label=Package Name&style=_shields_test')


Could you, just for the sake of a consistency, change an uri value to a yaml resource (from examples below)?

Whoops must've missed that one.

platan · 2018-03-30T09:36:03Z

services/yaml/yaml.tester.js

+  .get('.json?url=https://raw.githubusercontent.com/kubernetes/charts/568291d6e476c39ca8322c30c3f601d0383d4760/stable/coredns/Chart.yaml&query=$.version')
+  .expectJSONTypes(Joi.object().keys({
+    name: 'custom badge',
+    value: Joi.string().regex(/^\d+(\.\d+)?(\.\d+)?$/)


I think you can use an isSemver validator here:

shields/services/xml/xml.tester.js

Lines 55 to 57 in 66c5d91

.expectJSONTypes(Joi.object().keys({

name: 'custom badge',

value: isSemver

Or even better you can change expect to

.expectJSON({ name: 'custom badge', value: '0.8.0' });

since we are using resource from a specific commit.

I just copied the test from the json dynamic badge. Happy to update if you prefer.

platan · 2018-03-30T09:38:03Z

frontend/components/dynamic-badge-maker.js

@@ -42,6 +42,7 @@ export default class DynamicBadgeMaker extends React.Component {
          onChange={event => this.setState({ datatype: event.target.value })}>
          <option value="" disabled>data type</option>
          <option value="json">json</option>
+          <option value="yaml">yaml</option>


Can we sort them in an alphabetical order (json, xml, yaml)?

See other comment about order.

platan · 2018-03-30T09:41:41Z

services/yaml/yaml.tester.js

+
+t.create('YAML from url | with prefix & suffix & label')
+  .get('.json?url=https://raw.githubusercontent.com/kubernetes/charts/568291d6e476c39ca8322c30c3f601d0383d4760/stable/coredns/Chart.yaml&query=$.version&prefix=v&suffix= dev&label=Shields')
+  .expectJSONTypes(Joi.object().keys({


What do you thin about using this approach?

.expectJSON({ name: 'Shields', value: 'v0.8.0 dev' });

Much neater, again I just copied the other test.

platan · 2018-03-30T09:43:13Z

frontend/components/usage.js

@@ -123,6 +123,9 @@ export default class Usage extends React.PureComponent {
        <p>
          <code>/badge/dynamic/json.svg?url=&lt;URL&gt;&amp;label=&lt;LABEL&gt;&amp;query=&lt;<a href="https://www.npmjs.com/package/jsonpath" target="_BLANK" title="JSONdata syntax">$.DATA.SUBDATA</a>&gt;&amp;colorB=&lt;COLOR&gt;&amp;prefix=&lt;PREFIX&gt;&amp;suffix=&lt;SUFFIX&gt;</code>
        </p>
+        <p>
+          <code>/badge/dynamic/yaml.svg?url=&lt;URL&gt;&amp;label=&lt;LABEL&gt;&amp;query=&lt;<a href="https://www.npmjs.com/package/jsonpath" target="_BLANK" title="JSONdata syntax">$.DATA.SUBDATA</a>&gt;&amp;colorB=&lt;COLOR&gt;&amp;prefix=&lt;PREFIX&gt;&amp;suffix=&lt;SUFFIX&gt;</code>


Can we sort them in an alphabetical order (json, xml, yaml)?

Yes sure. I went with this order because the usage of json and yaml are the same and so thought I would keep them together.

jacobtomlinson · 2018-03-30T12:28:16Z

Thanks for the review comments @platan. I'll get this PR updated later on with the request changes.

jacobtomlinson · 2018-03-30T13:45:42Z

@platan I have made the changes!

platan · 2018-03-30T16:50:30Z

@jacobtomlinson thanks for fixes. That was quick! The code looks great in my opinion.

I have one extra thing. What would we like to see as a badge value when resource is not a valid yaml file? Something simple like invalid (https://github.com/badges/shields/blob/master/doc/service-tests.md#tutorial)? Currently we have such result for https://www.google.com/robots.txt
YAML:

JSON:

And for https://img.shields.io/badge/x-y-z.png
YAML:

JSON:

XML badge returns
for both resources.

@paulmelnikow @RedSparr0w What do you think about this?

jacobtomlinson · 2018-03-30T20:09:33Z

A consistent error message would be good. I think invalid would be fine but not sure if it accurately describes what the problem is. Perhaps something like invalid data source, parse error or load error would be more useful when troubleshooting.

Happy to go with whatever you think. Do you want me to implement it for all three dynamic badges in this PR or raise a new PR for it?

platan · 2018-03-31T06:47:25Z

@jacobtomlinson I agree with you, invalid data source or parse error is better than invalid. In my opinion we can merge this PR and prepare a separate one for handling parsing errors. Let's wait for opinion of the rest of the team.

RedSparr0w · 2018-03-31T22:39:49Z

Sounds good to me, I originally left it as the default error message so the user could try and fix the badge/problem themselves, but I think something like 'invalid json', 'invalid source', 'parse error' would work better

paulmelnikow · 2018-04-01T15:52:45Z

My preference would be invalid json / invalid xml / invalid yaml which is terse and consistent with other badges.

platan · 2018-04-01T16:35:42Z

@RedSparr0w or @paulmelnikow can you approve this PR? Then we could merge it and new PR with a better error message can be raised.

paulmelnikow

Left two minor comments. @platan feel free to give it the final 👍 and merge. Thanks!

paulmelnikow · 2018-04-02T01:43:09Z

frontend/lib/badge-url.spec.js

+  test(dynamicBadgeUrl, () => {
+    const yamlUrl = 'http://example.com/foo.yaml';
+    const query = '$.bar';
+    const prefix = 'value: ';


Aren't these three tests of dynamicBadgeUrl testing exactly the same thing? Maybe we should strip out two of them.

Good point. Let's remove two of them.

paulmelnikow · 2018-04-02T01:45:22Z

server.js

+      case 'yaml':
+        requestOptions = {
+          headers: {
+            Accept: 'application/yaml, text/yaml, text/plain'


From this Quora answer it seems we probably should use 'application/x-yaml, text/x-yaml, text/plain' here. Unless someone has a better source saying something else?

github.com, search in code:

"text/x-yaml" content type 39,389 code results

"text/yaml" content type 79,485 code results

"application/x-yaml" content type 34,264 code results

"application/yaml" content type 21,948 code results

Should we add all of them since it's not standardized?

Yes that's probably best.

jacobtomlinson · 2018-04-03T13:20:13Z

I have removed the extra tests and added the MIME types as suggested by @paulmelnikow.

I have also merged in the latest master and run npm install again to generate a new package-lock.json which resolves the merge conflicts it was complaining about.

Should be good to merge now!

platan · 2018-04-03T16:53:37Z

@jacobtomlinson thanks. Code looks OK. I have one doubt about package-lock.json. In all dependencies of fsevents "bundled" (https://docs.npmjs.com/files/package-lock.json#bundled) property is replaced with "resolved" and "integrity". I'm not sure if this change changes anything. 4 months ago we had change from "resolved"+"integrity" to "bundled" 0da212b.
@jacobtomlinson @paulmelnikow What do you think about this? Maybe this depends on a NPM version? Should we stick to a specific version of NPM?

jacobtomlinson · 2018-04-03T16:58:13Z

I don't feel I can add useful input here. To be honest this is the first project I've ever seen which tracks the package lock, I've always seen it in the `.gitignore`. Happy to change it however you wish.

jacobtomlinson · 2018-04-07T11:18:37Z

What action needs to happen here to get this merged?

platan · 2018-04-08T19:59:21Z

@jacobtomlinson Thanks for your contribution and your patience 👍 Would you like to prepare a next PR with an error message when data source is invalid? You could try with messages proposed by Paul (#1623 (comment)).

@RedSparr0w Thank you for a review.

Add dynamic yaml badge

dc934bf

Forgot package lock

5570949

paulmelnikow reviewed Mar 29, 2018

View reviewed changes

paulmelnikow changed the title ~~Add dynamic yaml badge~~ Add [dynamic-yaml] badge Mar 29, 2018

jacobtomlinson added 2 commits March 30, 2018 08:45

Switch tests to yaml data source

fdfa368

Add yaml to the dynamic badge maker options

3db5429

paulmelnikow requested review from platan and RedSparr0w March 30, 2018 07:52

Reorder to match documentation examples

fd824a4

platan reviewed Mar 30, 2018

View reviewed changes

jacobtomlinson added 3 commits March 30, 2018 14:40

Reordered dynamic types to be alphabetical

4cb5be3

Removed regex as pinend commit makes it unnecessary and fixed url

787bdc3

Removed unused import

bed3c08

chris48s mentioned this pull request Mar 30, 2018

[appveyor] Error handling in BaseService #1590

Merged

platan approved these changes Mar 31, 2018

View reviewed changes

paulmelnikow approved these changes Apr 2, 2018

View reviewed changes

paulmelnikow added the service-badge New or updated service badge label Apr 2, 2018

Add more YAML MIME types

864db17

jacobtomlinson added 2 commits April 3, 2018 14:16

Removed duplicate tests which don't differ between data types

df4f29e

Resolve conflicts by running npm install

c365c39

platan self-assigned this Apr 3, 2018

RedSparr0w approved these changes Apr 8, 2018

View reviewed changes

platan merged commit d56b696 into badges:master Apr 8, 2018

jacobtomlinson deleted the dynamic-yaml branch April 9, 2018 10:46

	.expectJSONTypes(Joi.object().keys({
	name: 'custom badge',
	value: isSemver

Add [dynamic-yaml] badge #1623

Add [dynamic-yaml] badge #1623

Conversation

jacobtomlinson commented Mar 29, 2018 • edited Loading

shields-ci commented Mar 29, 2018 • edited Loading

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

Choose a reason for hiding this comment

jacobtomlinson commented Mar 29, 2018

jacobtomlinson commented Mar 30, 2018

paulmelnikow commented Mar 30, 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

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

jacobtomlinson commented Mar 30, 2018

jacobtomlinson commented Mar 30, 2018

platan commented Mar 30, 2018

jacobtomlinson commented Mar 30, 2018

platan commented Mar 31, 2018

RedSparr0w commented Mar 31, 2018

paulmelnikow commented Apr 1, 2018

platan commented Apr 1, 2018

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jacobtomlinson commented Apr 3, 2018

platan commented Apr 3, 2018

jacobtomlinson commented Apr 3, 2018 via email • edited Loading

jacobtomlinson commented Apr 7, 2018

platan commented Apr 8, 2018

jacobtomlinson commented Mar 29, 2018 •

edited

Loading

shields-ci commented Mar 29, 2018 •

edited

Loading

jacobtomlinson commented Apr 3, 2018 via email •

edited

Loading