Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Disable /probot redirect #1498

Closed
stephenmathieson opened this issue Apr 1, 2021 · 8 comments
Closed

Disable /probot redirect #1498

stephenmathieson opened this issue Apr 1, 2021 · 8 comments

Comments

@stephenmathieson
Copy link

Feature Request

Is your feature request related to a problem? Please describe.

When upgrading from Probot v10 to v11, I cannot disable/bypass the / to /probot redirect. We have a few apps that use the / route for documentation.

Describe the solution you'd like

A way for my app to respond with custom HTML for GET / requests.

Using Probot v10, this worked fine:

module.exports = app => {
  app.router.get('/', (req, res) => res.sendFile('./landing-page.html'))
  app.on('some_event', /* ... */)
}

However, with Probot v11, I cannot find a way to do this.

Describe alternatives you've considered

I've tried hacking server.expressApp._router.stack, but this seems dangerous. Additionally, the handler which performs the redirect is an anonymous function, so there's no way to safely remove it (cannot use Layer#name to identify it).

Teachability, Documentation, Adoption, Migration Strategy

I can think of a few ways to do this:

  • add a "name" to the redirect route, enabling it to be removed by walking server.expressApp._router.stack
  • add an option to not mount the redirect route
  • give users a way to access the underlying Express app prior to Probot mounting its route handlers (enabling user provided route handlers to take priority)

Note: I noticed that the WIP app does this in /api/index.js, but I have not been able to figure out what is calling/importing that file.

Note x 2: I'd be more than happy to work on implementing this if you let me know how you'd like to solve it.

@gr2m
Copy link
Contributor

gr2m commented Apr 1, 2021

Note: I noticed that the WIP app does this in /api/index.js, but I have not been able to figure out what is calling/importing that file.

The WIP app is deployed as serverless application to vercel.com. Vercel is controlling the routing, not the WIP app.

Did you try using the new routing API?

https://probot.github.io/docs/http/

Probot
GitHub Apps to automate and improve your workflow

@stephenmathieson
Copy link
Author

Did you try using the new routing API?

Yes, but the redirect takes priority over it. Here's a reproduction/test case:

module.exports = (app, { getRouter }) => {
  var router = getRouter('/')
  router.get('/', (req, res) => {
    res.send('Hello world')
  })
}

@gr2m
Copy link
Contributor

gr2m commented Apr 1, 2021

Okay, thanks for checking.

In that case, I'd run your own server: https://probot.github.io/docs/development/#use-codeservercode. It's not as well documented as it could be, but basically with v11 it's possible to "eject" probot and use the underlying APIs, until you get to point where you can make probot do what you need (and not do what you don't need). That way we don't need to keep adding features for edge cases to Probot, instead we can document how to do what you need using the underlying APIs if your use case is outside probot's core scope

Could you see if that works for you? I can help you out if you get stuck. If you could take notes and help improve the docs as you figure things out, that'd be much appreciated

Probot
GitHub Apps to automate and improve your workflow

@stephenmathieson
Copy link
Author

stephenmathieson commented Apr 1, 2021

That's fine, I can setup a custom server. Thanks.

Is it really that "edge-casey" to want / to resolve to something other than a redirect to /probot tho?

We have a requirement that we cannot leak implementation details of how the application works (mainly because of security concerns), meaning if someone stumbles upon the URL and sees "This app was built with Probot", we'd be in trouble.

@gr2m
Copy link
Contributor

gr2m commented Apr 1, 2021

It's not necessary an edge case for GitHub apps, but it's out of scope for the probot run ./index.js script. I think we should remove adding custom routes altogether, because at this point you really want control of your own server.

We have a requirement that we cannot leak implementation details of how the application works

Even more reason to maintain your own server. createNodeMiddleware() might be the best level of abstraction, if you are building an express server. You can createProbot() to continue the configuration using environment variables

@stephenmathieson
Copy link
Author

I think we should remove adding custom routes altogether, because at this point you really want control of your own server.

This makes sense to me.

I'll mess with createNodeMiddleware() over the weekend and see if I can get something working. Thanks for the advice 👍

@gr2m
Copy link
Contributor

gr2m commented Jul 9, 2021

closing in favor of #1576

@sommaichaisit
Copy link

Feature Request

Is your feature request related to a problem? Please describe.

When upgrading from Probot v10 to v11, I cannot disable/bypass the / to /probot redirect. We have a few apps that use the / route for documentation.

Describe the solution you'd like

A way for my app to respond with custom HTML for GET / requests.

Using Probot v10, this worked fine:

module.exports = app => {
  app.router.get('/', (req, res) => res.sendFile('./landing-page.html'))
  app.on('some_event', /* ... */)
}

However, with Probot v11, I cannot find a way to do this.

Describe alternatives you've considered

I've tried hacking server.expressApp._router.stack, but this seems dangerous. Additionally, the handler which performs the redirect is an anonymous function, so there's no way to safely remove it (cannot use Layer#name to identify it).

Teachability, Documentation, Adoption, Migration Strategy

I can think of a few ways to do this:

* add a "name" to the redirect route, enabling it to be removed by walking `server.expressApp._router.stack`

* add an option to not mount the redirect route

* give users a way to access the underlying Express app _prior_ to Probot mounting its route handlers (enabling user provided route handlers to take priority)

Note: I noticed that the WIP app does this in /api/index.js, but I have not been able to figure out what is calling/importing that file.

Note x 2: I'd be more than happy to work on implementing this if you let me know how you'd like to solve it.

Ss

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants