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

Add ACKEE_BASEURL configuration. #317

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

dodedodo
Copy link

@dodedodo dodedodo commented Jan 28, 2022

Hey @electerious,

I took a jab at implementing some sort of base URL support. Can you give this a test run and tell me what you think?

I wrote some documentation as well.

Base url

Change the url path at which Ackee listens. You can use this in conjunction with a reverse proxy like nginx or traefik to publish ackee on https://example.com/ackee/ instead of https://ackee.example.com/

ACKEE_BASEURL=/ackee

Do not add a trailing /.

One downside of this implementation is that with ACKEE_BASEURL=/ackee the server listens on example.com/ackee/ , but not at example.com/ackee . This is because the relative paths need the trailing / (at least, I think so. Couldn't get it to work otherwise). It's not a breaking bug, and it's easily solved by configuring a http redirect from /ackee to /ackee/ in your reverse proxy. Maybe we could include that redirect directly in server.js?

The above is now fixed.

@vercel
Copy link

vercel bot commented Jan 28, 2022

Someone is attempting to deploy a commit to a Personal Account owned by @electerious on Vercel.

@electerious first needs to authorize it.

@dodedodo dodedodo mentioned this pull request Jan 28, 2022
@dodedodo
Copy link
Author

I've added an automatic redirect from /baseurl to /baseurl/. This fixes my initial caveat described above.

I also noticed I forgot to update the healthcheck.js url's.

@dodedodo
Copy link
Author

dodedodo commented Feb 9, 2022

Hello @electerious, have you had time to check my work? If there's anything I can do to get this pull accepted please let me know 👍 .

I also just saw the other pull request for a base URL option (#288). I honestly did not see that before. I'm not trying to step on anyone toes, and I'm sure either of these pulls will do the job just fine 🙂.

src/server.js Outdated Show resolved Hide resolved
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

Successfully merging this pull request may close these issues.

None yet

2 participants