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

New configuration system #3589

Merged
merged 8 commits into from Sep 30, 2012
Merged

New configuration system #3589

merged 8 commits into from Sep 30, 2012

Conversation

jhass
Copy link
Member

@jhass jhass commented Sep 18, 2012

Follow up for #3585

About

EnvironmentConfiguration and AppConfig is replaced by a unified more flexible system. application.yml is now diaspora.yml and completely restructured, grouping related settings in categories and giving better names to some settings.

All settings can now be specified and be overridden by environment variables, the conversion scheme is explained at the top of the diaspora.yml example. My experience with Heroku is still low but I hope this makes it possible to do a complete transparent heroku configuration without special hacks in the code.

Draft for new Heroku guide is here: http://pad.spored.de/p/heroku

Environment variable changes

deprectated

  • REDISTOGO_URL in favour of REDIS_URL or ENVIRONMENT_REDIS

removed

  • application_yml - Obsolete, all settings are settable via environment variables now

renamed

  • SINGLE_PROCESS_MODE -> ENVIRONMENT_SINGLE_PROCESS_MODE
  • SINGLE_PROCESS -> ENVIRONMENT_SINGLE_PROCESS_MODE
  • NO_SSL -> ENVIRONMENT_REQUIRE_SSL
  • ASSET_HOST -> ENVIRONMENT_ASSETS_HOST

Cavehats

Using a proxy object to allow for access to nested settings leads to one major problem I'm not really happy with: Though Rubys awesome metaprogramming capabilities there are only two things false in the Ruby wold, NilClass and FalseClass. Even subclasses of these classes are true. This leads two major issues: if you put the proxy object into an if it's always true, and since it's always true the nil || 'default' construct doesn't work anymore. For the first problem my workaround is to check if the called method ends with ? and return the target of the proxy if so, this yields constructs like if AppConfig.mails_enabled? working, while if AppConfig.mails_enabled being always true. For the || construct I haven't found another workaround than calling the getter of the proxy object.

Todo

  • Specs ✔
  • Docs ✔
  • Cleanup unused settings ✔
  • Unify pod url/uri handling in the whole code ✔
  • Investigate/unify/finish migration to roles aka what the heck is that: community spotlight ✔
  • Update or remove lib/tasks/stats.rake to new role system ✔
  • Make asset_sync integration nice ✔
  • Update wiki (new heroku guide is linked above).
  • Lots of testing if this actually works everywhere as it should - feels good already, third party testers always welcome :)
  • Feedback :)

@jhass
Copy link
Member Author

jhass commented Sep 20, 2012

Woah green xD

@jhass
Copy link
Member Author

jhass commented Sep 20, 2012

almost done, now the hard part: deploy it to a test pod and setup a Heroku install with it :D

I'm not stopping anyone from doing the latter for me… just sayin'

@jaywink
Copy link
Contributor

jaywink commented Sep 20, 2012

Wow, awesome. Could maybe have time Sunday or next week to set up a Heroku install with this. Seems to me that this is so big that longer testing is needed anyway.

Is there an automatic migration from application.yml on install?

@jhass
Copy link
Member Author

jhass commented Sep 20, 2012

Nope and I guess it would add another 300 lines of code, so I'll leave it for now. Time to force everybody to some fresh configfiles anyway :P.

Thanks for the heroku stuff, when you try it please try not to create a diaspora.yml (will add a commit to make it possible) and only change the settings that you want to differ from the defaults via heroku config:add :)

@jhass
Copy link
Member Author

jhass commented Sep 21, 2012

I took a stab at the heroku pod myself, but now I immediately get a 500 without any sign of an exception, I tried piping every possible logging to stdout but no signs :/ Here's my rewritten heroku guide for the new configuration system: http://pad.spored.de/p/heroku

If anyone has any clue what else to try…

@jhass
Copy link
Member Author

jhass commented Sep 21, 2012

Okay I figured it out and updated everything, now to S3…

@jhass
Copy link
Member Author

jhass commented Sep 22, 2012

Squashed, rebased and ready for serious review :)

@jaywink
Copy link
Contributor

jaywink commented Sep 25, 2012

Updated our Heroku minipod (cbtpod.herokuapp.com) first to develop head and then merged this in.

Everything seems to work ok except smtp sending issue but that was not working on our pod even before the update.

Would it be too much to add a migration script (just a shell script helper) that would copy the values from application.yml to diaspora.yml? I imagine this would help many many pod owners when it becomes time to update.

@jhass
Copy link
Member Author

jhass commented Sep 25, 2012

If someone contributes one I likely won't reject it, but our current development focus is all about removing old cruft and restructuring everything to a cleaner codebase, that migration script would be cruft in no time.

@jhass
Copy link
Member Author

jhass commented Sep 25, 2012

Oh and thanks for testing :)

@jaywink
Copy link
Contributor

jaywink commented Sep 25, 2012

Yeah I guess that is a good point.

@Raven24
Copy link
Member

Raven24 commented Sep 26, 2012

from the heroku draft document I get how env variables are supposed to look like, but I guess a general description of how configuration by env vars and the schema for their names and a few examples should go in the wiki or probably the changelog, too.

Maybe a change this large even justifies a guest-post on the devblog or perhaps a separate page on the wiki... anyway, I think we should not make the same mistake and let the docs limp behind the development of the code, as it was often the case in the past.
Also, as a change where podmin interaction is required, this should be announced on as many channels as possible (mailing lists, diaspora hq account, devblog...)

but apart from that I think this is a great improvement on the old system and it provides a whole lot more flexibility.

@jhass
Copy link
Member Author

jhass commented Sep 26, 2012

Hm, I do explain the conversion scheme at the top of diaspora.yml.example, do you think it would be enough to point from the changelog to it? Should I expand the explanation there a bit more? As a programmer I'm not a fan of repeating myself :P

Regarding docs: Yeah they surely need updating but I'm unsure when to do it. Ideally it would need to be done in the same moment we do a release, but where to prepare the changes?

For the announcement: We should just broadcast releases in general and include such massive changes in there, no need for two broadcasts IMO. Release announcements for sure should take place on the MLs, the blogs (I'd like to get rid if the devblog and have only one, but that's another story), Diaspora HQ and I don't know, whatever else is available.

Thanks :)

@jaywink
Copy link
Contributor

jaywink commented Sep 26, 2012

What happens if a podmin updates the code without reading the announcement?
Will the pod just fail to run?

On 26 September 2012 17:16, Jonne Haß notifications@github.com wrote:

Hm, I do explain the conversion scheme at the top of diaspora.yml.example,
do you think it would be enough to point from the changelog to it? Should I
expand the explanation there a bit more?

Regarding docs: Yeah they surely need updating but I'm unsure when to do
it. Ideally it would need to be done in the same moment we do a release,
but where to prepare the changes?

For the announcement: We should just broadcast releases in general and
include such massive changes in there, no need for two broadcasts IMO.
Release announcements for sure should take place on the MLs, the blogs (I'd
like to get rid if the devblog and have only one, but that's another
story), Diaspora HQ and I don't know, whatever else is available.

Thanks :)


Reply to this email directly or view it on GitHubhttps://github.com//pull/3589#issuecomment-8891238.

@Raven24
Copy link
Member

Raven24 commented Sep 26, 2012

Ok gotta admit, the env var explanation in diaspora.yml was tl;dr, so I had no idea you already did explain it. Maybe an additional pointer in the Changelog wouldn't hurt.

The docs are kinda ... stupid now. I'd rather we generate the more technical parts from the comments inside the source code with rdoc or doxygen (like rails). Then we'd have docs that are coupled to the code releases...

@jaywink yeah, pretty much.
see: github.com//pull/3589/files#L73R17

@jhass
Copy link
Member Author

jhass commented Sep 26, 2012

So two people without major issue, I'd say I'll squash the last four commits into the main one and then we merge and do further fixing in develop? :)

@jaywink
Copy link
Contributor

jaywink commented Sep 26, 2012

Sounds good to me. Pity the documentation cannot be pre-edited and then
just published, easily anyway.

On 26 September 2012 17:49, Jonne Haß notifications@github.com wrote:

So two people without major issue, I'd say I'll squash the last four
commits into the main one and then we merge and do further fixing in
develop? :)


Reply to this email directly or view it on GitHubhttps://github.com//pull/3589#issuecomment-8892397.

@Raven24
Copy link
Member

Raven24 commented Sep 26, 2012

+1

* Throw away old system
* Add new system
* Add new example files
* Replace all calls
* add the most important docs
* Add Specs
* rename disable_ssl_requirement to require_ssl
* cloudfiles isn't used/called in our code
* since community_spotlight.list is only used as enable flag replace it with such one and remove all legacy and irelevant codepaths around it
* die if session secret is unset and on heroku
* First basic infrastructure for version information
@jaywink
Copy link
Contributor

jaywink commented Sep 28, 2012

I think I might have found a bug :P Been trying to disable invites on our company internal pod (which runs develop + this pull) and couldn't find a way to do it. But in current master it is possible at least by setting "open_invitations: false" and "invite_count: 0".

Can anyone confirm?

@jaywink
Copy link
Contributor

jaywink commented Sep 28, 2012

Just to confirm I have these set in diaspora.yml:

'''

## Settings about invitations
invitiations:

  ## Set this to true if you want users to invite as many
  ## people as they want.
  open: false

  ## The default amount of invitiations an invite link has.
  ## Every user has such a link. Only counts if open is false.
  count: 0

'''

@jhass
Copy link
Member Author

jhass commented Sep 28, 2012

Whoops, totally my bad, see the commit I added :)

@jaywink
Copy link
Contributor

jaywink commented Sep 28, 2012

:D haha managed to totally miss that too ;) tnx

@jhass
Copy link
Member Author

jhass commented Sep 30, 2012

So, do I really need to merge my own stuff? :P

@Raven24
Copy link
Member

Raven24 commented Sep 30, 2012

yes of course, we need someone to blame, in case it doesn't work :P

Raven24 added a commit that referenced this pull request Sep 30, 2012
New configuration system, details: see changelog
@Raven24 Raven24 merged commit 8678c14 into diaspora:develop Sep 30, 2012
@Raven24
Copy link
Member

Raven24 commented Sep 30, 2012

@DeadSuperHero with this merged we're almost approaching the 0.0.1 milestone here on github.
I'd say get ready to announce a "RELEASE" sometime this week ;)

@hfase01
Copy link
Contributor

hfase01 commented Oct 1, 2012

Omg wow, I have been pulling my hair out for a couple hours chasing a syntax error after converting to "diaspora.yml"...

Line 26 looked to me like something that needed to be set.. Sooo I put something there; (environment: 'production')
and that caused rake to fail on a syntax error.

rake aborted!
syntax error on line 15, col 4: `    url: 'https://hfase.com/''

Tasks: TOP => db:migrate => environment
(See full trace by running task with --trace)

If those are just category headings or somthing can we make them look diffrent than the actual settings?
Ea. "ENVIROMENT" instead of "enviroment:" ?? Just seems really confusing to have those look like that if nothing is meant to be set there. :)

@jhass
Copy link
Member Author

jhass commented Oct 1, 2012

hm that would need some special casing in the code I wouldn't like to be honest, we could just add YAML group markers (&something) or add some comments above them, what do you think? On the other hand the top states that comments are prefixed with ## and settings with # by default.

@jhass
Copy link
Member Author

jhass commented Oct 1, 2012

How about a simple ## Group after each section?

environment: ## Group

@hfase01
Copy link
Contributor

hfase01 commented Oct 1, 2012

LOL I guess I should have read it a little slower. Been a loooong night..

Not sure what to suggest though. :/ Maybe just specifically mentioning those in the preface would be enough really. :)
After all, not everyone might be as impatient as me. LOL Your preface is excellent though!

I hope I didn't sound angered or anything, The new config system you guys are working on is beautiful!!

Just my luck though I run into something again that has misleading errors LOL I was thinking it was the URL line....

@hfase01
Copy link
Contributor

hfase01 commented Oct 1, 2012

Crap, [OT] - (cant figure out how to quote, wish github was more forum-like...)

Your second post there Jonne would work perfect! Just anything simple to make that stand out a little. :)

@jhass
Copy link
Member Author

jhass commented Oct 1, 2012

Thanks, will do it that way. Btw. y u no ask on IRC? :P

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

5 participants