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 more config options via script #2

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
2 participants
@SISheogorath
Copy link
Member

SISheogorath commented Sep 26, 2016

Building an own container to allow custom configs is not really nice. So let's bring the config options to environment variables.

This should help to simplify the setup process a lot. (Please notice that there is currently no documentation and also not every config option possible)

Currently possible:

  • DOMAIN
  • URLPATH
  • PROTOCOLUSESSL
  • URLADDPORT
  • USECDN
  • GITHUB_CLIENTID
  • GITHUB_CLIENTSECRET
  • TWITTER_CLIENTID
  • TWITTER_CLIENTSECRET
  • FACEBOOK_CLIENTID
  • FACEBOOK_CLIENTSECRET
  • GITLAB_BASEURL
  • GITLAB_CLIENTID
  • GITLAB_CLIENTSECRET

Feel free to extend ;)

@jackycute

This comment has been minimized.

Copy link
Member

jackycute commented Sep 26, 2016

@Sheogorath-SI Nice work! This is great for advance configuration.
There is a option called "urlpath", could you add that too?
https://github.com/hackmdio/hackmd/blob/master/lib/config.js

@SISheogorath

This comment has been minimized.

Copy link
Member

SISheogorath commented Sep 26, 2016

Done. ;)

@SISheogorath

This comment has been minimized.

Copy link
Member

SISheogorath commented Sep 26, 2016

Oh I just saw that things like DOMAIN and URL_PATH are already captured by config.js in hackmd itself. may should change the parameter. or remove because it's not needed. The whole handling is a bit weird.

@jackycute

This comment has been minimized.

Copy link
Member

jackycute commented Sep 26, 2016

@Sheogorath-SI Ahhh, you're right. I missed what I have written.

@jackycute

This comment has been minimized.

Copy link
Member

jackycute commented Sep 26, 2016

@jackycute

This comment has been minimized.

Copy link
Member

jackycute commented Sep 26, 2016

Another good approach is make these environment variables available in the config.js.
What do you think?

@SISheogorath

This comment has been minimized.

Copy link
Member

SISheogorath commented Sep 26, 2016

I'm unsure. Should we replace/override these variables or leave them as they are and remove them from configure.js?

The nameschema is a bit weird because URL_PATH looks "wrong" :D

And yes, may the config.js is the better place if we want to make it easier to dockerize the whole application

@SISheogorath

This comment has been minimized.

Copy link
Member

SISheogorath commented Sep 26, 2016

My first intention was about fixing the docker containers. fixing the application itself is the right and better way. But using common variables like DOMAIN and URL_PATH looks bad to me in case of common setups. should be something like HACKMD_DOMAIN. Especially because the environment variables are preferred against the config file.

@jackycute

This comment has been minimized.

Copy link
Member

jackycute commented Sep 26, 2016

I understand, but should also make some capability with old variables.
And I'm not sure how other application would handle these naming.
here is the example of gitlab https://docs.gitlab.com/ce/ci/variables/README.html

@jackycute

This comment has been minimized.

Copy link
Member

jackycute commented Sep 26, 2016

OK, I will make these changes to config.js on next release.
Please be patient, I'm a bit busy in this week.

@SISheogorath

This comment has been minimized.

Copy link
Member

SISheogorath commented Sep 26, 2016

No problem :) sounds great.

My setup is working right now so no need to hurry

@jackycute

This comment has been minimized.

Copy link
Member

jackycute commented Oct 11, 2016

@jackycute

This comment has been minimized.

Copy link
Member

jackycute commented Oct 11, 2016

Added in 0.4.5

@jackycute jackycute closed this Oct 11, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment