[QueryStringStatic] service #3024

mbarkhau · 2019-02-18T09:12:43Z

This is WIP (no updated documentation yet) and is based on PR #3012

shields-ci · 2019-02-18T09:13:33Z

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

Generated by 🚫 dangerJS against 8e310ce

paulmelnikow

Thanks! I left some preliminary comments.

core/badge-urls/make-badge-url.js

paulmelnikow · 2019-02-18T15:41:00Z

services/static-badge/query-string-static.service.js

+    }
+  }
+
+  handle(namedParams, queryParams) {


It would be good to validate the queryParams using Joi, as we've done in e.g. the dynamic badge. It will ensure that message is a string or numeric string, and not e.g. an array. (same issue as #2854)

I added validation as best as I could figure out how. I'm sure there's a better way.

This works for now! That code is in flight in #3042 so if that lands first, we can update this; otherwise after this lands I can update #3042.

paulmelnikow · 2019-02-18T15:43:13Z

services/static-badge/query-string-static.tester.js

+  .get(
+    '/static/v1.svg?label=label&message=message&color=notacolor&style=_shields_test'
+  )
+  .expectJSON({ name: 'label', value: 'message', color: null })


These seem like they are testing functionality in coalesceBadge + BaseService. There's already a "set color" test at the bottom, so how about removing all of these?

If it seems important to add more tests of that code I would prefer we find a way to do it with a unit test instead of a service test, as the unit tests get run more frequently.

Fair point.

paulmelnikow · 2019-02-18T15:43:57Z

services/static-badge/query-string-static.tester.js

+  .get(
+    '/static/v1.svg?label=best-license&message=Apache-2.0&color=blue&style=_shields_test'
+  )
+  .expectJSON({ name: 'best-license', value: 'Apache-2.0', color: 'blue' })


The "Shields-encoded dash" is the unusual escaping used in the url-based static badge. It doesn't seem relevant here and could be removed.

Good point!

paulmelnikow · 2019-02-18T23:01:19Z

I updated the PR title so the service tests will run starting with the next build.

mbarkhau · 2019-02-19T08:43:19Z

So I guess the next step here is to update the shields.io page and add documentation for this. Should it be more like the endpoint badge, on a separate page, or should it be a widget similar to the current static badge, or something else?

Maybe the most simple thing would be to add another output url on the current static widget.

paulmelnikow · 2019-02-19T14:37:31Z

Maybe the most simple thing would be to add another output url on the current static widget.

Yea, seems fine to do the simple thing for now.

That text is in flight in #3032 so perhaps best to get that merged first.

core/badge-urls/make-badge-url.js

paulmelnikow · 2019-02-19T14:42:10Z

services/static-badge/query-string-static.service.js

+    }
+  }
+
+  handle(namedParams, queryParams) {


This works for now! That code is in flight in #3042 so if that lands first, we can update this; otherwise after this lands I can update #3042.

mbarkhau · 2019-02-21T10:42:45Z

3aca1ef adds a snippet on the front page, which looks like this

I'm not sure how you want to treat the new static urls vs the old ones. With the commit f9b5c20 I've updated the fronted to use the /static/v1.svg urls everywhere, which may be a bit much, idk.

In order to keep the history linear I usually like to do git rebase and assuming nobody else is working on this branch I then do git push --force. Since this branch has merge conflicts with master again, how would you like that to be resolved? Should I merge master to this branch and give up on the linear history?

paulmelnikow · 2019-02-21T14:32:24Z

In order to keep the history linear I usually like to do git rebase and assuming nobody else is working on this branch I then do git push --force. Since this branch has merge conflicts with master again, how would you like that to be resolved? Should I merge master to this branch and give up on the linear history?

We keep PRs to single units of work and use a squash merge workflow, and as a result can optimize the history in PRs for people who are following the PRs. So please merge master into your branch, and we'll squash it down when it's done.

paulmelnikow · 2019-02-21T14:37:23Z

Thanks for carrying this forward!

3aca1ef adds a snippet on the front page, which looks like this

Looks good!

I'd like to ship a second version of the URL path version that is a bit friendlier. But this feature is intended for (1) easy programmatic generation and (2) complicated text that is otherwise hard to escape. So, could you put the new text below instead of above?

Also I don't think there's a need to label it beta. We've been thinking about this for a long time and I'm feeling confident in the changes you're making. 😁

I'm not sure how you want to treat the new static urls vs the old ones. With the commit f9b5c20 I've updated the fronted to use the /static/v1.svg urls everywhere, which may be a bit much, idk.

Again some of the code you're changing is in flight in #3032; so apologies, but I think we need to land that first. I'd lean toward using the old badge everywhere else because the URLs are more readable than query strings. If we get a more streamlined URL-path version down the line we could switch them over.

mbarkhau · 2019-02-21T15:00:12Z

I'd lean toward using the old badge everywhere else because the URLs are more readable than query strings.

Sounds reasonable. I'll revert f9b5c20.

This reverts commit f9b5c20.

mbarkhau · 2019-02-21T16:20:18Z

I swapped the ordering of the URLs and removed the "(beta)" text.

Anything else this needs before a merge?

paulmelnikow · 2019-02-21T16:45:59Z

It looks good! Did you merge in the changes from #3032? That should make it quick to merge this after that lands.

The query param changes landed in #3042; I'll leave a comment about that.

services/static-badge/query-string-static.service.js

mbarkhau · 2019-02-21T18:37:36Z

This PR is now pending on the merge of #3032 since I merged consolidate-badge-gen.

paulmelnikow · 2019-02-21T20:39:30Z

Thanks for the updates! Will approve and merge when #3032 is in.

shields-deployment · 2019-02-22T02:12:27Z

This pull request was merged to master branch. This change is now waiting for deployment, which will usually happen within a few days. Stay tuned by joining our #ops channel on Discord!

After deployment, changes are copied to gh-pages branch:

paulmelnikow reviewed Feb 18, 2019

View reviewed changes

Consolidate badge URL generation functions

95e8d35

paulmelnikow changed the title ~~Query string static service~~ [QueryStringStatic] service Feb 18, 2019

mbarkhau force-pushed the query-string-static-service branch from 45a9b72 to f0c5679 Compare February 19, 2019 06:33

paulmelnikow reviewed Feb 20, 2019

View reviewed changes

Merge branch 'master' into consolidate-badge-gen

85294af

mbarkhau force-pushed the query-string-static-service branch from e5f36ff to e8e19ea Compare February 20, 2019 08:17

mbarkhau and others added 10 commits February 20, 2019 09:28

add make-badge-url fuction for query-string-static

cbd9d05

add query-string-static service

9e697b9

no default color in url, use backend default

59e9729

remove redundant tests

274f456

fix service tests for query-string-static

8359330

add minimal joi validation

b10e99d

update test for query-string-static error

7d71281

Fix typo

5782d4e

remove schemaVersion param from queryStringStaticBadgeUrl

a823764

prettier (old habits die hard)

2ca144c

mbarkhau force-pushed the query-string-static-service branch from e8e19ea to 2ca144c Compare February 20, 2019 08:29

paulmelnikow and others added 4 commits February 20, 2019 19:28

Merge branch 'master' into consolidate-badge-gen

a518c05

use consistent names in frontend\n\nsubject -> label\nstatus -> message

3f83c8f

add snippet for static/v1

3aca1ef

change most frontend badges to /static/v1 urls

f9b5c20

mbarkhau and others added 2 commits February 21, 2019 16:05

Revert "change most frontend badges to /static/v1 urls"

473c677

This reverts commit f9b5c20.

Merge branch 'master' into consolidate-badge-gen

e4932b6

paulmelnikow mentioned this pull request Feb 21, 2019

Consolidate badge URL generation functions #3032

Merged

mbarkhau added 2 commits February 21, 2019 16:57

change order of static url in usage.js

2b0433d

Merge branch 'master' into query-string-static-service

17e5024

mbarkhau marked this pull request as ready for review February 21, 2019 16:18

paulmelnikow reviewed Feb 21, 2019

View reviewed changes

services/static-badge/query-string-static.service.js Outdated Show resolved Hide resolved

mbarkhau added 3 commits February 21, 2019 18:05

use queryParamSchema validation from badges#3032

e771a06

Merge branch 'consolidate-badge-gen' into query-string-static-service

cfd61c1

fix test for changed error message

e5c93fe

Merge branch 'master' into query-string-static-service

8e310ce

paulmelnikow approved these changes Feb 22, 2019

View reviewed changes

paulmelnikow merged commit f6628e6 into badges:master Feb 22, 2019

chris48s mentioned this pull request Feb 21, 2024

Website does not produce expected static badge outputs #9852

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[QueryStringStatic] service #3024

[QueryStringStatic] service #3024

mbarkhau commented Feb 18, 2019 •

edited by paulmelnikow

Loading

shields-ci commented Feb 18, 2019 •

edited

Loading

paulmelnikow left a comment

paulmelnikow Feb 18, 2019

mbarkhau Feb 19, 2019

paulmelnikow Feb 19, 2019

paulmelnikow Feb 18, 2019

mbarkhau Feb 19, 2019

paulmelnikow Feb 18, 2019

mbarkhau Feb 18, 2019

paulmelnikow commented Feb 18, 2019

mbarkhau commented Feb 19, 2019

paulmelnikow commented Feb 19, 2019

paulmelnikow Feb 19, 2019

mbarkhau commented Feb 21, 2019

paulmelnikow commented Feb 21, 2019

paulmelnikow commented Feb 21, 2019

mbarkhau commented Feb 21, 2019

mbarkhau commented Feb 21, 2019 •

edited

Loading

paulmelnikow commented Feb 21, 2019

mbarkhau commented Feb 21, 2019

paulmelnikow commented Feb 21, 2019

shields-deployment bot commented Feb 22, 2019

[QueryStringStatic] service #3024

[QueryStringStatic] service #3024

Conversation

mbarkhau commented Feb 18, 2019 • edited by paulmelnikow Loading

shields-ci commented Feb 18, 2019 • 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

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 commented Feb 18, 2019

mbarkhau commented Feb 19, 2019

paulmelnikow commented Feb 19, 2019

Choose a reason for hiding this comment

mbarkhau commented Feb 21, 2019

paulmelnikow commented Feb 21, 2019

paulmelnikow commented Feb 21, 2019

mbarkhau commented Feb 21, 2019

mbarkhau commented Feb 21, 2019 • edited Loading

paulmelnikow commented Feb 21, 2019

mbarkhau commented Feb 21, 2019

paulmelnikow commented Feb 21, 2019

shields-deployment bot commented Feb 22, 2019

mbarkhau commented Feb 18, 2019 •

edited by paulmelnikow

Loading

shields-ci commented Feb 18, 2019 •

edited

Loading

mbarkhau commented Feb 21, 2019 •

edited

Loading