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

Selfhosted version improvements #89

Merged
merged 112 commits into from Jul 2, 2020
Merged

Selfhosted version improvements #89

merged 112 commits into from Jul 2, 2020

Conversation

tckb
Copy link
Contributor

@tckb tckb commented Jun 15, 2020

This PR will address some of the concerns raised during the self-hosted versions:

closes #26, closes #85, closes #86, closes #88, closes #133

  • cleanup docker-compose
  • separate migration step
  • remove/skip plausible.io landing page for self-hosting setup
  • optionally enable registration page
  • optionally enable sign-in page
  • add tests
  • remove landing page and other unused templates
  • remove Joken dependenc
  • fix the app page to show login/register instead of "trial period"
  • fix config param inconsistency issue and invalid value to param - closes Setting db poolsizes for postgres and clickhouse gives me a kernel dum #198

tckb added 30 commits May 4, 2020 20:29
Signed-off-by: Chandra Tungathurthi <tckb@tgrthi.me>
Signed-off-by: Chandra Tungathurthi <tckb@tgrthi.me>
Signed-off-by: Chandra Tungathurthi <tckb@tgrthi.me>
Signed-off-by: Chandra Tungathurthi <tckb@tgrthi.me>
Signed-off-by: Chandra Tungathurthi <tckb@tgrthi.me>
Signed-off-by: Chandra Tungathurthi <tckb@tgrthi.me>
…testing

Signed-off-by: Chandra Tungathurthi <tckb@tgrthi.me>
Signed-off-by: Chandra Tungathurthi <tckb@tgrthi.me>
Gitlab Migration: Adding test stage

See merge request tckb-public/plausible!1
Signed-off-by: Chandra Tungathurthi <tckb@tgrthi.me>
Signed-off-by: Chandra Tungathurthi <tckb@tgrthi.me>
Signed-off-by: Chandra Tungathurthi <tckb@tgrthi.me>
Signed-off-by: Chandra Tungathurthi <tckb@tgrthi.me>
Signed-off-by: Chandra Tungathurthi <tckb@tgrthi.me>
Signed-off-by: Chandra Tungathurthi <tckb@tgrthi.me>
Signed-off-by: Chandra Tungathurthi <tckb@tgrthi.me>
Signed-off-by: Chandra Tungathurthi <tckb@tgrthi.me>
Signed-off-by: Chandra Tungathurthi <tckb@tgrthi.me>
Signed-off-by: Chandra Tungathurthi <tckb@tgrthi.me>
Signed-off-by: Chandra Tungathurthi <tckb@tgrthi.me>
Added Dockerfile and cleaned up configurations

See merge request tckb-public/plausible!2
Signed-off-by: Chandra Tungathurthi <tckb@tgrthi.me>
Signed-off-by: Chandra Tungathurthi <tckb@tgrthi.me>
Adding overlay scripts for Db migration  on release artifacts

See merge request tckb-public/plausible!3
Signed-off-by: Chandra Tungathurthi <tckb@tgrthi.me>
Signed-off-by: Chandra Tungathurthi <tckb@tgrthi.me>
Signed-off-by: Chandra Tungathurthi <tckb@tgrthi.me>
Signed-off-by: Chandra Tungathurthi <tckb@tgrthi.me>
tckb added 5 commits June 19, 2020 22:01
Resolve "Cosmetic changes to self-hosted version"

Closes plausible#20

See merge request tckb-public/plausible!25
- added ability for disabling
  - authentication completely
  - registration
  - landing page

- formatting cleanups
Improvements to selfhosting setup

See merge request tckb-public/plausible!26
@tckb tckb marked this pull request as ready for review June 20, 2020 19:55
tckb added 2 commits June 21, 2020 10:39
- added ability for disabling
  - authentication completely
  - registration
  - landing page

- formatting cleanups
added test cases

See merge request tckb-public/plausible!27
Signed-off-by: Chandra Tungathurthi <tckb@tgrthi.me>
Copy link
Contributor

@ukutaht ukutaht left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me! Thanks for the huge work 💚 💙 💜 💛 ❤️

The only change I'd like to see is to remove the DISABLE_LANDING_PAGE variable. Some context: the / route is never hit in the cloud version of Plausible. We started out with it as the landing page but now we're running a static site in front of the app server with a reverse proxy. The static site shadows the / route to serve the landing page.

This means the / route is not even relevant to our cloud deployment. In a self-hosted scenario, its should probably:

  • Redirect to /login if the user is not logged in
  • Redirect to /sites if the user is logged in

Seems like that's what the DISABLE_LANDING_PAGE already does so you could just remove the env variable do it unconditionally.

@tckb
Copy link
Contributor Author

tckb commented Jun 25, 2020

Looks good to me! Thanks for the huge work 💚 💙 💜 💛 ❤️

The only change I'd like to see is to remove the DISABLE_LANDING_PAGE variable. Some context: the / route is never hit in the cloud version of Plausible. We started out with it as the landing page but now we're running a static site in front of the app server with a reverse proxy. The static site shadows the / route to serve the landing page.

This means the / route is not even relevant to our cloud deployment. In a self-hosted scenario, its should probably:

* Redirect to `/login` if the user is not logged in

* Redirect to `/sites` if the user is logged in

Seems like that's what the DISABLE_LANDING_PAGE already does so you could just remove the env variable do it unconditionally.

I thought about that, but since it being a opensource, I do not want to rule out a possibility that someone with a fork want to customize the landing page and would still want have a landing page. we can set this variable to true by default.

I would probably would want to customize it somehow -- What do you think ?

@tckb tckb requested a review from ukutaht June 25, 2020 09:16
@ukutaht
Copy link
Contributor

ukutaht commented Jun 25, 2020

I still don't see much reason to keep it tbh. If someone has a fork and they're adding code for a landing page, it shouldn't be a big ask to also change the Pagecontroller.Index handler to render the landing page instead of doing redirection.

Instead of referencing an old landing page we don't use anymore, it would be better to reference the code for our live landing page.

The main reason I want to remove it is because our CSS is compiled based on the classes used in the actual HTML templates. By removing the landing page we remove some of Tailwind's CSS classes from being included in the final build. This would make the whole app lighter.

@tckb
Copy link
Contributor Author

tckb commented Jun 25, 2020

The main reason I want to remove it is because our CSS is compiled based on the classes used in the actual HTML templates. By removing the landing page we remove some of Tailwind's CSS classes from being included in the final build. This would make the whole app lighter.

Okay, I agree on the size part. So removing the templates at https://github.com/plausible/analytics/tree/master/lib/plausible_web/templates/page would suffice right?

@ukutaht
Copy link
Contributor

ukutaht commented Jun 25, 2020

Okay, I agree on the size part. So removing the templates at https://github.com/plausible/analytics/tree/master/lib/plausible_web/templates/page would suffice right?

Yeah, and changing the controller to redirect based on the login state here:
https://github.com/plausible/analytics/blob/master/lib/plausible_web/controllers/page_controller.ex#L25

@tckb
Copy link
Contributor Author

tckb commented Jun 26, 2020

Yeah, and changing the controller to redirect based on the login state here:

This is already handled by auto_auth_plug.

I will remove unnecessary assets and templates to needed for self-hosting

@ukutaht
Copy link
Contributor

ukutaht commented Jun 26, 2020

Cool 👍 I will merge once those are removed

@ukutaht
Copy link
Contributor

ukutaht commented Jun 26, 2020

One more thing, let's make the landing page disabled by default.

@tckb
Copy link
Contributor Author

tckb commented Jun 26, 2020

One more thing, let's make the landing page disabled by default.

If I remove the template, then there wouldn't be any landing page :P I am not sure if I understand this correctly

tckb added 2 commits June 28, 2020 10:53
Signed-off-by: Chandra Tungathurthi <tckb@tgrthi.me>
…ter'

Resolve "Remove landing page and  unsed templates"

Closes plausible#21

See merge request tckb-public/plausible!28
tckb added 2 commits June 30, 2020 20:01
Signed-off-by: Chandra Tungathurthi <tckb@tgrthi.me>
…ter'

More fixes and addressing PR comments

Closes plausible#21

See merge request tckb-public/plausible!29
@tckb tckb requested a review from ukutaht June 30, 2020 20:08
@ukutaht ukutaht merged commit 517d4db into plausible:master Jul 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants