Added basic support for [hockeyapp] #2055

TobiasRoeddiger · 2018-09-05T20:57:49Z

Hello. I added basic support for a badge for Hockeyapp deployments for iOS and Android. Thank you for the awesome project!

shields-ci · 2018-09-05T21:00:08Z

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

Generated by 🚫 dangerJS against 9da8e48

…into hockeyapp

chris48s

Thanks for submitting this. All the major stuff is already looking good. I've left some comments to look at but most of them should be quite small points.

chris48s · 2018-09-05T21:31:43Z

services/hockeyapp/hockeyapp.service.js

+  async handle({ apptoken, appid }) {
+    const json = await this.fetch({ apptoken, appid })
+
+    if (json.app_versions === undefined || json.app_versions.length === 0) {


Your schema says that the app_versions key must be an array.
How does the situation where json.app_versions === undefined occur?
Also, given there are some custom criteria here for the 'not found' response, can we have service tests covering these cases please?

The undefined check is just a sanity check but as it will be checked by Joi anyways I will remove the undefined check and rely on that. Also adding the test case you mentioned.

chris48s · 2018-09-05T21:32:02Z

services/hockeyapp/hockeyapp.service.js

+      version = latestVersionObject.version
+    }
+
+    return this.constructor.render({ version: version })


Minor style point: This could just be return this.constructor.render({ version })

chris48s · 2018-09-05T21:32:30Z

services/hockeyapp/hockeyapp.service.js

+  static get examples() {
+    return [
+      {
+        exampleUrl: 'hockeyapp/simple',


I've set up a staging deploy for this at https://shields-staging-pr-2055.herokuapp.com/#/ and this example give me an invalid message: https://shields-staging-pr-2055.herokuapp.com/hockeyapp/v/hockeyapp/simple.svg Can you take a look. The example should return a valid response

Same issue here as with the test case that hits the live api - which account should I use for this example?

chris48s · 2018-09-05T21:33:32Z

services/hockeyapp/hockeyapp.tester.js

+
+t.create('Invalid AppToken or AppId')
+  .get('hockeyapp/v/1234/1234.json')
+  .expectJSONTypes({ name: 'hockeyapp', value: 'invalid' })


If we're just comparing to an object literal, we use expectJSON() in preference to expectJSONTypes() - it gives us a more meaningful error message if it fails :)

chris48s · 2018-09-05T21:34:26Z

services/hockeyapp/hockeyapp.tester.js

+t.create('Invalid AppToken or AppId')
+  .get('hockeyapp/v/1234/1234.json')
+  .expectJSONTypes({ name: 'hockeyapp', value: 'invalid' })
+


We should have at least one valid case test in the suite which hits the live API and doesn't mock the response. I realise it is unusual practice for unit tests to depend on an external service ("don't test what you don't own") but we integrate with hundreds of services. If an upstream API changes or goes away, we need a test to start failing on the daily build otherwise we won't notice it.

I totally understand that but we would have to rely on an app token that has to be obtained with a real account at https://hockeyapp.net - how do you handle this situation?

chris48s · 2018-09-06T19:31:08Z

Thanks for having a look at those comments.

I totally understand that but we would have to rely on an app token that has to be obtained with a real account at https://hockeyapp.net - how do you handle this situation?

Same issue here as with the test case that hits the live api - which account should I use for this example?

Any real app token would be fine (e.g: one of your own). Something that is stable is preferred :) an app you plan to delete in the near future would be an unhelpful choice. By the looks of it, you can generate an app token with read-only permissions. See:

https://support.hockeyapp.net/kb/api/api-basics-and-authentication#authentication-api

Just to understand better (I had not heard of this service until reviewing this PR so I don't know the in's and out's of it - you'll have to bring me up to speed), would be there be a negative consequence to a user of exposing a read-only app token? e.g: would that token allow someone to also access sensitive information about your account? If so, this would probably not be a suitable contribution for shields.io (although you might want to run it on a self-hosted instance). We shouldn't encourage our users to expose a token which allows access to protected info in their public readme.

TobiasRoeddiger · 2018-09-06T20:41:59Z

Thank you for getting back. As you have mentioned it all depends on the scope of the app token (e.g. just set it to the scope of a specific app and read only) which would make it perfectly sufficient to expose the token publicly. At least as far as I can tell by the docs. I'm looking into the options of how to make the example endpoint stable and will update the PR accordingly.

chris48s · 2018-09-06T20:57:28Z

OK, thanks. Given it is important for users to use the correct token scope here, lets make this a little bit more clear. I'd suggest 2 things:

Lets rename the param to :read-only-token
When we specify a token we can specify an optional documentation key. Lets use that to explain that this should be a read-only key (and link to the relevant docs).

e.g:

{
  exampleUrl: '78e9ae0287ca4a07b2cbbb1338e1c71b/7e53b1f0ebe171be1b9004ff04a2f663',
  urlPattern: ':read-only-token/:appid',
  staticExample: this.render({ version: '1.0.0' }),
  documentation: "bla bla some extra info here"
}

chris48s · 2018-09-06T21:00:10Z

services/hockeyapp/hockeyapp.tester.js

@@ -25,7 +25,7 @@ t.create('Android App')
        ],
      })
  )
-  .expectJSONTypes({ name: 'hockeyapp', value: isVPlusDottedVersionAtLeastOne })
+  .expectJSON({ name: 'hockeyapp', value: isVPlusDottedVersionAtLeastOne })


Just to clarify my previous comment:

We use .expectJSON() when comparing to an object literal e.g: .expectJSON({ name: 'hockeyapp', value: 'invalid' }) but .expectJSONTypes() is necessary when we're validating against an expression, so these ones still need to be .expectJSONTypes(). You'll probably find these are failing.

Sorry this is a bit fiddly

Thanks for clarifying. Will fix this.

TobiasRoeddiger · 2018-09-07T08:19:33Z

I'm a bit confused about those tests failing, because the requests work on my local machine after trying them manually. I see them failing when I run them locally, too.

chris48s · 2018-09-07T16:20:53Z

Aah. The reason the service test fail is because when you create the test object with createServiceTester() it is already automatically appending /hockeyapp on to the base URL for you. If you change your .get() calls from .get('hockeyapp/v/1234/1234.json') and so on to .get('/v/1234/1234.json') that should get your tests running as intended.

I'm aware this is currently not well documented. As you've probably noticed, we're currently transitioning from one architecture to another. I finished updating the tutorial earlier this week. I'm planning to work on updating the service test docs next week.

Latest changes are looking good. If you can get those tests passing and add the additional case which hits a live endpoint without mocking, this should be good to go 👍 .

chris48s · 2018-09-30T20:21:38Z

@TobiasRoeddiger - are you interested in finishing this? I think there is probably only the minor fix to get the service tests passing in order to get this merged.

TobiasRoeddiger · 2018-10-19T13:16:15Z

@TobiasRoeddiger - are you interested in finishing this? I think there is probably only the minor fix to get the service tests passing in order to get this merged.

Yes I am! Sorry but had some really important things to do and this just slipped out of scope.

paulmelnikow · 2019-01-03T14:56:49Z

@TobiasRoeddiger Would be great to get this merged! Would you be up for wrapping this up?

paulmelnikow · 2019-01-07T23:42:17Z

Would anyone else like to take a few moments to wrap this up?

TobiasRoeddiger · 2019-01-08T10:40:48Z

I'm really sorry. npm run test is currently failing for me and it would be great if you can point me towards what is going wrong.

1) The server
       should produce colorscheme PNG badges:

      AssertionError: expected <Buffer 3c 21 64 6f 63 74 79 70 65 20 68 74 6d 6c 3e 3c 6d 65 74 61 20 63 68 61 72 73 65 74 3d 75 74 66 2d 38 3e 3c 74 69 74 6c 65 3e 41 20 73 65 72 76 65 72 ... 155 more bytes> to satisfy [Function]
      + expected - actual

      -false
      +true
      
      at Context.<anonymous> (server.spec.js:41:35)
      at process.internalTickCallback (internal/process/next_tick.js:77:7)

  2) The CLI
       should produce PNG badges:
     ChildProcessError: `node lib/badge-cli.js cactus grown .png` failed with code 1
      at ChildProcess.<anonymous> (node_modules/child-process-promise/lib/index.js:132:23)
      at ChildProcess.EventEmitter.emit (domain.js:441:20)
      at maybeClose (internal/child_process.js:978:16)
      at Socket.stream.socket.on (internal/child_process.js:395:11)
      at Socket.EventEmitter.emit (domain.js:441:20)
      at Pipe._handle.close (net.js:616:12)

  3) The rasterizer
       should produce PNG:
     Error: Stream yields empty buffer
      at Socket.<anonymous> (node_modules/gm/lib/command.js:57:17)
      at Socket.EventEmitter.emit (domain.js:441:20)
      at endReadableNT (_stream_readable.js:1098:12)
      at process.internalTickCallback (internal/process/next_tick.js:72:19)

  4) The rasterizer
       should cache its results:
     Error: Stream yields empty buffer
      at Socket.<anonymous> (node_modules/gm/lib/command.js:57:17)
      at Socket.EventEmitter.emit (domain.js:441:20)
      at endReadableNT (_stream_readable.js:1098:12)
      at process.internalTickCallback (internal/process/next_tick.js:72:19)



npm ERR! code ELIFECYCLE
npm ERR! errno 4
npm ERR! [email protected] test:js:server: `HANDLE_INTERNAL_ERRORS=false mocha "*.spec.js" "lib/**/*.spec.js" "services/**/*.spec.js"`
npm ERR! Exit status 4
npm ERR! 
npm ERR! Failed at the [email protected] test:js:server script.
npm ERR! This is probably not a problem with npm. There is likely additional logging output above.

npm ERR! A complete log of this run can be found in:
npm ERR!     /Users/tobiasroddiger/.npm/_logs/2019-01-08T10_38_29_433Z-debug.log
npm ERR! code ELIFECYCLE
npm ERR! errno 4
npm ERR! [email protected] test: `npm run lint && npm run test:js:frontend && npm run test:js:server`
npm ERR! Exit status 4
npm ERR! 
npm ERR! Failed at the [email protected] test script.
npm ERR! This is probably not a problem with npm. There is likely additional logging output above.

npm ERR! A complete log of this run can be found in:
npm ERR!     /Users/tobiasroddiger/.npm/_logs/2019-01-08T10_38_29_461Z-debug.log

paulmelnikow · 2019-01-08T15:26:27Z

Installing imagemagick should fix that. If it doesn’t you could also safely ignore or those few tests, or describe.skip them.

calebcartwright · 2019-01-09T01:24:59Z

services/hockeyapp/hockeyapp.service.js

+    return 'version'
+  }
+
+  static get url() {


this signature should now be static get route()

calebcartwright · 2019-01-09T01:27:40Z

I'm really sorry. npm run test is currently failing for me and it would be great if you can point me towards what is going wrong.

Given that you're only modifying service code here, you may want to just run a npm run test:services -- --only=hockeyapp to just run the service tests for your code.

calebcartwright · 2019-01-09T01:32:33Z

services/hockeyapp/hockeyapp.service.js

+  async handle({ apitoken, appid }) {
+    const json = await this.fetch({ apitoken, appid })
+
+    if (json.app_versions.length === 0) {


I think you could move this check to the schema instead:

app_versions: Joi.array() .items( Joi.Object({ ... }) ) .min(1), ...

calebcartwright · 2019-01-09T01:34:35Z

services/hockeyapp/hockeyapp.service.js

+  static get url() {
+    return {
+      base: 'hockeyapp/v',
+      format: '(.+)/(.+)',


We can switch this to use pattern now: remove the format and capture keys and replace them with:

pattern: ':apioken+/:appid+'

that's assuming those two params both need the full (.+) capture group

paulmelnikow · 2019-01-17T02:21:08Z

Hey @TobiasRoeddiger would you have a moment to address these comments? It would be great to get this shipped! 🚀

paulmelnikow · 2019-01-21T17:08:14Z

Feel free to reopen this when you have time! And, same goes for anyone else who would like to pick this up. You can grab the commits from this branch, merge in latest master, address the comments, and open a new PR.

TobiasRoeddiger and others added 3 commits September 5, 2018 22:32

added basic support for hockeyapp

11777eb

added more test cases

13bd32a

Merge branch 'master' into hockeyapp

c131cd1

TobiasRoeddiger added 4 commits September 5, 2018 23:02

added version checking also fixing unused var error

d5a32d4

Merge branch 'hockeyapp' of https://github.com/TobiasRoeddiger/shields …

7366a3b

…into hockeyapp

npm prettier

128f43d

fixed small spelling error

f6cf480

chris48s reviewed Sep 5, 2018

View reviewed changes

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

TobiasRoeddiger added 3 commits September 5, 2018 23:53

removed useless version tag identifier and switched to expectJSON

0910196

prettier

aea26a7

added test case for not found edge case

edb757e

paulmelnikow temporarily deployed to shields-staging-pr-2055 September 5, 2018 22:04 Inactive

made sample url less verbose

78134a0

paulmelnikow temporarily deployed to shields-staging-pr-2055 September 5, 2018 22:07 Inactive

npm prettier

52fe489

paulmelnikow temporarily deployed to shields-staging-pr-2055 September 5, 2018 22:08 Inactive

added working example

8790364

paulmelnikow temporarily deployed to shields-staging-pr-2055 September 6, 2018 20:49 Inactive

prettier

09e166a

paulmelnikow temporarily deployed to shields-staging-pr-2055 September 6, 2018 20:50 Inactive

chris48s changed the title ~~Added basic support for Hockeyapp~~ Added basic support for [hockeyapp] Sep 6, 2018

chris48s reviewed Sep 6, 2018

View reviewed changes

TobiasRoeddiger added 2 commits September 7, 2018 10:02

fixed expectJSON

43de47a

changed apptoken to api-token and added a description

680bb91

paulmelnikow deployed to shields-staging-pr-2055 September 7, 2018 08:12 View deployment

Merge branch 'master' into hockeyapp

1d76817

paulmelnikow had a problem deploying to shields-staging-pr-2055 January 3, 2019 14:57 Failure

Merge branch 'master' into hockeyapp

9da8e48

paulmelnikow had a problem deploying to shields-staging-pr-2055 January 9, 2019 01:18 Failure

calebcartwright reviewed Jan 9, 2019

View reviewed changes

paulmelnikow added the on-hold Deferred in favor of another approach, blocked on preceding efforts, stale, or abandoned label Jan 21, 2019

paulmelnikow closed this Jan 21, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added basic support for [hockeyapp] #2055

Added basic support for [hockeyapp] #2055

TobiasRoeddiger commented Sep 5, 2018

shields-ci commented Sep 5, 2018 •

edited

Loading

chris48s left a comment

chris48s Sep 5, 2018

TobiasRoeddiger Sep 5, 2018

chris48s Sep 5, 2018

chris48s Sep 5, 2018

TobiasRoeddiger Sep 5, 2018 •

edited

Loading

chris48s Sep 5, 2018

chris48s Sep 5, 2018

TobiasRoeddiger Sep 5, 2018 •

edited

Loading

chris48s commented Sep 6, 2018

TobiasRoeddiger commented Sep 6, 2018 •

edited

Loading

chris48s commented Sep 6, 2018

chris48s Sep 6, 2018

TobiasRoeddiger Sep 6, 2018

TobiasRoeddiger commented Sep 7, 2018 •

edited

Loading

chris48s commented Sep 7, 2018

chris48s commented Sep 30, 2018

TobiasRoeddiger commented Oct 19, 2018

paulmelnikow commented Jan 3, 2019

paulmelnikow commented Jan 7, 2019

TobiasRoeddiger commented Jan 8, 2019

paulmelnikow commented Jan 8, 2019 •

edited

Loading

calebcartwright Jan 9, 2019

calebcartwright commented Jan 9, 2019

calebcartwright Jan 9, 2019

calebcartwright Jan 9, 2019

paulmelnikow commented Jan 17, 2019

paulmelnikow commented Jan 21, 2019

Added basic support for [hockeyapp] #2055

Added basic support for [hockeyapp] #2055

Conversation

TobiasRoeddiger commented Sep 5, 2018

shields-ci commented Sep 5, 2018 • 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

TobiasRoeddiger Sep 5, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TobiasRoeddiger Sep 5, 2018 • edited Loading

Choose a reason for hiding this comment

chris48s commented Sep 6, 2018

TobiasRoeddiger commented Sep 6, 2018 • edited Loading

chris48s commented Sep 6, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TobiasRoeddiger commented Sep 7, 2018 • edited Loading

chris48s commented Sep 7, 2018

chris48s commented Sep 30, 2018

TobiasRoeddiger commented Oct 19, 2018

paulmelnikow commented Jan 3, 2019

paulmelnikow commented Jan 7, 2019

TobiasRoeddiger commented Jan 8, 2019

paulmelnikow commented Jan 8, 2019 • edited Loading

Choose a reason for hiding this comment

calebcartwright commented Jan 9, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paulmelnikow commented Jan 17, 2019

paulmelnikow commented Jan 21, 2019

shields-ci commented Sep 5, 2018 •

edited

Loading

TobiasRoeddiger Sep 5, 2018 •

edited

Loading

TobiasRoeddiger Sep 5, 2018 •

edited

Loading

TobiasRoeddiger commented Sep 6, 2018 •

edited

Loading

TobiasRoeddiger commented Sep 7, 2018 •

edited

Loading

paulmelnikow commented Jan 8, 2019 •

edited

Loading