/probot homepage #279

JasonEtco · 2017-10-08T18:23:34Z

As we discussed in #262, it would be really cool to have something if someone visits the / path of a running Probot App. @gr2m had a good suggestion to have it redirect to /probot if there isn't any other set route, which makes a lot of sense. Unfortunately, that creates the challenge of somehow identifying if another route handler for / has in fact been created.

This PR adds a veeeeeery basic /probot page (needs work), and the relevant tests to:

Make sure that /probot responds correctly
Make sure that / redirects to /probot
/ can be overwritten to no longer redirect to /probot.

1-2 work perfectly, but 3 is a challenge. I've passed one of two relevant tests:

If the newer .get() is registered against the Probot App's server, it'll work just fine. However, if the new route is registered as part of a new express.Router then it is totally ignored 😭 This is made clear in @anglinb's test from #255, which I've copied into this PR.

I'd appreciate some help here; I'm stuck on how else to identify if a path has been registered across both the central express app and any nested routers. I thought of writing some methods into the robot class that would add new routes to an array and then checking that, but that adds a lot of complication around just writing routes.

Let me know if you have any questions 💖 Note that this PR is WIP because ~~I'm stuck and need help 🍕~~ I want your opinions on the way I'm solving this.

UPDATE:

I've found a way to overwrite the / route by creating a Set() of routes, and adding all routes in new routers (as a part of the load() method) to the Set. Then, when / is requested, it checks the Set to see if it has a route. If it does, next(), otherwise redirect to /probot.

@example

* origin/master: chore: Read version from package.json feat: Expose public stats about installations (#181) chore: remove changelog now that we are using semantic-release chore: update jsdoc to fix error party machen chore(travis): Run test on node 8 chore: semantic-release setup Move note for robot.log before the @example Added NPM version badge to readme. Changes made in CONTRIBUTING.md according to #271 Update output mentioned in 'Generating a new app' section Smaller image in the welcome PR comment docs: fix log statement Lint JS in examples Whitespace Add @example for using defaultConfig

BREAKING CHANGE: `probot run` without any arguments will no longer autoload apps named `probot-*`.

* refactor-loading: Simplify resolver Update simulate command Look for `apps` key instead of `plugins` Move setup Remove autoloading of plugings Refactor app loading # Conflicts: # index.js

* origin/intro-web-page: Remove duplicate load

bkeepers

@JasonEtco I merged in #284 extracted the default homepage to a built-in app. The diff should look a lot better once #284 is merged.

The page is looking pretty good. It'd be nice to link to documentation and maybe even the slack channel to point people toward places where they can find help building their app.

JasonEtco · 2017-10-13T22:33:55Z

@bkeepers that's a great idea, I'll do that. What's the timeline looking like for #284? Can I do anything to help it get merged?

You had a real productive ✈️ ride huh?

JasonEtco · 2017-10-14T12:44:51Z

/probot as of my latest commit:

* origin/master: chore(tests): Migrate to Jest chore(docs): Use GH_TOKEN for updating docs # Conflicts: # package-lock.json

bkeepers

See installation stats

Since this is just a JSON dump right now, what would you think about removing this for now and revisiting in another PR that adds an HTML view of the installation stats?

JasonEtco · 2017-10-14T20:46:17Z

@bkeepers that's not a bad idea, but EVEN BETTER IDEA: what about including those numbers in the page? Or is that too much for a basic welcome page? Could be done by including the /plugins/stats.js methods into the welcome page route and putting it into the .ejs template, or could have an in-browser fetch to the /probot/stats endpoint.

gr2m · 2017-10-16T22:38:00Z

@JasonEtco I’d work/discuss showing stats in a follow up PR, this one is quite big already

JasonEtco · 2017-10-16T22:38:51Z

Yeah I agree @gr2m. In that case, I'll pull down changes, fix CI and merge away?

gr2m · 2017-10-16T22:46:09Z

I tested this PR on one of my Probot Test Apps on Glitch, but got this error

Error: Failed to lookup view "probot.ejs" in views directory "/app/views"
    at Function.render (/app/node_modules/express/lib/application.js:580:17)
    at ServerResponse.render (/app/node_modules/express/lib/response.js:1008:7)
    at app.get (/app/node_modules/probot/lib/plugins/default.js:14:40)
    at Layer.handle [as handle_request] (/app/node_modules/express/lib/router/layer.js:95:5)
    at next (/app/node_modules/express/lib/router/route.js:137:13)
    at Route.dispatch (/app/node_modules/express/lib/router/route.js:112:3)
    at Layer.handle [as handle_request] (/app/node_modules/express/lib/router/layer.js:95:5)
    at /app/node_modules/express/lib/router/index.js:281:22
    at Function.process_params (/app/node_modules/express/lib/router/index.js:335:12)
    at next (/app/node_modules/express/lib/router/index.js:275:10)
    at Function.handle (/app/node_modules/express/lib/router/index.js:174:3)
    at router (/app/node_modules/express/lib/router/index.js:47:12)
    at Layer.handle [as handle_request] (/app/node_modules/express/lib/router/layer.js:95:5)
    at trim_prefix (/app/node_modules/express/lib/router/index.js:317:13)
    at /app/node_modules/express/lib/router/index.js:284:7
    at Function.process_params (/app/node_modules/express/lib/router/index.js:335:12)
    at next (/app/node_modules/express/lib/router/index.js:275:10)
    at /app/node_modules/express/lib/router/index.js:635:15
    at next (/app/node_modules/express/lib/router/index.js:260:14)
    at Function.handle (/app/node_modules/express/lib/router/index.js:174:3)
    at router (/app/node_modules/express/lib/router/index.js:47:12)
    at Layer.handle [as handle_request] (/app/node_modules/express/lib/router/layer.js:95:5)
    at trim_prefix (/app/node_modules/express/lib/router/index.js:317:13)
    at /app/node_modules/express/lib/router/index.js:284:7
    at Function.process_params (/app/node_modules/express/lib/router/index.js:335:12)
    at Immediate.next (/app/node_modules/express/lib/router/index.js:275:10)
    at Immediate._onImmediate (/app/node_modules/express/lib/router/index.js:635:15)
    at runCallback (timers.js:802:20)
    at tryOnImmediate (timers.js:762:5)
    at processImmediate [as _immediateCallback] (timers.js:733:5)

Are you still looking into it? Code wise it looks ok, but CI is failing, too. Let me know if you get stuck or when the PR is ready for review

JasonEtco · 2017-10-16T22:49:01Z

@gr2m I'm not seeing that error locally, but I'm getting some different CI errors. I'm fairly certain the functionality itself works, its just a testing thing to figure out. I'll let you know if I'm having trouble, thanks 💖

JasonEtco · 2017-10-16T23:08:14Z

It looks like the last failing test is due to the new .setup() method. Here's the error message I see:

 FAIL  test/index.js
  ● Probot › server › redirects to /probot from /

    secretOrPrivateKey must have a value

      at Object.<anonymous>.module.exports [as sign] (node_modules/jsonwebtoken/sign.js:70:20)
      at generateJwt (node_modules/github-app/index.js:39:16)
      at Object.asApp (node_modules/github-app/index.js:7:54)
      at Robot.auth (lib/robot.js:148:31)
      at getInstallations (lib/plugins/stats.js:29:32)
      at refresh (lib/plugins/stats.js:22:33)
      at Object.<anonymous>.module.exports (lib/plugins/stats.js:9:26)
      at load (index.js:79:5)
      at apps.concat.forEach.app (index.js:86:45)
          at Array.forEach (<anonymous>)
      at Object.setup (index.js:86:30)
      at Object.it (test/index.js:47:14)
          at Promise (<anonymous>)
          at <anonymous>
      at process._tickDomainCallback (internal/process/next_tick.js:228:7)

I'm guessing it has something to do with authenticating the probot instance, but I'm having trouble nailing down the fix. Any ideas @gr2m? cc @bkeepers.

By the by @gr2m I did fix your ejs bug in the latest commit, thanks for pointing it out.

JasonEtco · 2017-10-17T00:21:33Z

Aha! It seems to be the /stats endpoint that throws when first creating an authed GitHub client and attempting to ping the installation API as a part of .setup(). I'm not sure of the best way to resolve this; commenting out the require for the stats plugin in the defaultApps array fixes this test case, but obviously that's not a real solution.

JasonEtco · 2017-10-17T01:38:00Z

Alrighty, per a Slack conversation, it turns out that the reason the last failing test was failing isn't related to this PR and should not be a blocker.

The TL;DR of it is that we are missing a way of mocking Certs and things for default plugins, which was causing the stats plugin to throw an error. While this should probably be resolved, the particular test that I've removed was unrelated.

I think this is good to merge 🎉 Can I get one last look over @gr2m, @bkeepers, @probot/maintainers?

bkeepers

Sorry, my fault for breaking the tests. I'm not sure why it wasn't failing for me locally.

JasonEtco added 4 commits October 8, 2017 11:19

Add /probot route with tests

47e91ef

Add index route, write tests

af3e985

Remove unused arg

fc30d6d

Pass tests

202ed4b

JasonEtco requested review from anglinb, bkeepers and gr2m October 8, 2017 19:11

JasonEtco and others added 10 commits October 8, 2017 16:43

Tweak HTML string and pkg require

d55db01

Add static directory

3ab4c7b

Design welcome page

0779880

Rename to probot.html

cdb18b3

Implement EJS

eb3ebe5

Remove duplicate load

cd00b00

Extract default homepage into plugin

011768b

Fix issue after merge conflict

9f21cd0

Refactor app loading

3f8bd7e

bkeepers mentioned this pull request Oct 12, 2017

Refactor app loading #284

Merged

bkeepers added 11 commits October 12, 2017 12:45

Remove autoloading of plugings

85bcad0

BREAKING CHANGE: `probot run` without any arguments will no longer autoload apps named `probot-*`.

Move setup

0c4ebf1

Look for apps key instead of plugins

53bfc44

Update simulate command

655f1ed

Simplify resolver

53cf420

Merge branch 'refactor-loading' into probot-intro-web-page

5ca2de0

* refactor-loading: Simplify resolver Update simulate command Look for `apps` key instead of `plugins` Move setup Remove autoloading of plugings Refactor app loading # Conflicts: # index.js

Remove unused routes variable

cd78bef

Move views to default path

80f3e66

Call probot.setup to properly initialize default apps

57d20d3

Simplify package loading

18aa948

Merge remote-tracking branch 'origin/intro-web-page' into intro-web-page

a805422

* origin/intro-web-page: Remove duplicate load

bkeepers reviewed Oct 13, 2017

View reviewed changes

Specify extension

7e5cd00

JasonEtco added 2 commits October 14, 2017 05:32

Merge in master

cf97c16

Add links to docs/slack

61b2a6b

JasonEtco changed the title ~~WIP: /probot homepage~~ /probot homepage Oct 14, 2017

This was referenced Oct 14, 2017

return app name, version and description at root path when opened in browser #254

Closed

Adds test to show that root path can be overwritten #255

Closed

Merge remote-tracking branch 'origin/master' into probot-intro-web-page

f13b9bf

* origin/master: chore(tests): Migrate to Jest chore(docs): Use GH_TOKEN for updating docs # Conflicts: # package-lock.json

bkeepers reviewed Oct 14, 2017

View reviewed changes

JasonEtco added 2 commits October 16, 2017 18:40

Remove stats link

a710bd1

Add helper to jest ignore

4ed5ca0

Fix ejs views error

dc4c0b8

JasonEtco added 2 commits October 16, 2017 21:29

Move a require to top of file

2cf994a

Remove unnecessary (and breaking) test

fdf3a07

bkeepers approved these changes Oct 17, 2017

View reviewed changes

JasonEtco merged commit 5d29945 into master Oct 17, 2017

JasonEtco deleted the intro-web-page branch October 17, 2017 13:36

JasonEtco mentioned this pull request Nov 14, 2017

Return something helpful on GET / when opened in the browser #253

Closed

johnperez416 mentioned this pull request Sep 3, 2023

[Snyk] Upgrade pino-http from 5.5.0 to 8.4.0 johnperez416/probot#149

Closed

johnperez416 mentioned this pull request Nov 11, 2023

[Snyk] Upgrade pino-http from 5.5.0 to 8.5.0 johnperez416/probot#160

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

/probot homepage #279

/probot homepage #279

JasonEtco commented Oct 8, 2017 •

edited

bkeepers left a comment

JasonEtco commented Oct 13, 2017

JasonEtco commented Oct 14, 2017

bkeepers left a comment

JasonEtco commented Oct 14, 2017

gr2m commented Oct 16, 2017

JasonEtco commented Oct 16, 2017 •

edited

gr2m commented Oct 16, 2017

JasonEtco commented Oct 16, 2017

JasonEtco commented Oct 16, 2017

JasonEtco commented Oct 17, 2017 •

edited

JasonEtco commented Oct 17, 2017

bkeepers left a comment

/probot homepage #279

/probot homepage #279

Conversation

JasonEtco commented Oct 8, 2017 • edited

UPDATE:

bkeepers left a comment

Choose a reason for hiding this comment

JasonEtco commented Oct 13, 2017

JasonEtco commented Oct 14, 2017

bkeepers left a comment

Choose a reason for hiding this comment

JasonEtco commented Oct 14, 2017

gr2m commented Oct 16, 2017

JasonEtco commented Oct 16, 2017 • edited

gr2m commented Oct 16, 2017

JasonEtco commented Oct 16, 2017

JasonEtco commented Oct 16, 2017

JasonEtco commented Oct 17, 2017 • edited

JasonEtco commented Oct 17, 2017

bkeepers left a comment

Choose a reason for hiding this comment

JasonEtco commented Oct 8, 2017 •

edited

JasonEtco commented Oct 16, 2017 •

edited

JasonEtco commented Oct 17, 2017 •

edited