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

Support for Meteor app settings (non-bundled mode) is broken #1403

Closed
tgoorden opened this issue Mar 4, 2015 · 29 comments

Comments

Projects
None yet
5 participants
@tgoorden
Copy link

commented Mar 4, 2015

As it stands, setting Meteor.settings with the METEOR_SETTINGS environment is not supported (anymore?):

meteor/meteor#2005

Since there is currently no way to pass --settings options to a Passenger Meteor app, this means that this crucial Meteor mechanism is broken. --settings is important to have "environment dependent" variables, with the added bonus that they can be loaded from outside the htdocs directory.

@FooBarWidget

This comment has been minimized.

Copy link
Member

commented Mar 5, 2015

You're right. I'll schedule this for 5.0.2.

For now, you can patch this locally by editing the file helper-scripts/meteor-loader.rb. Look for:

exec("meteor run -p #{port} #{production}")

Add any --settings you want.

@FooBarWidget FooBarWidget added this to the 5.0.2 milestone Mar 5, 2015

@FooBarWidget

This comment has been minimized.

Copy link
Member

commented Mar 5, 2015

@tgoorden So how exactly does --settings work in when your Meteor app is bundled? Does it accept value from an environment variable? If so, does that environment variable work too when the Meteor app is not bundled?

@FooBarWidget FooBarWidget modified the milestones: 5.0.2, 5.0.3 Mar 7, 2015

@FooBarWidget

This comment has been minimized.

Copy link
Member

commented Mar 10, 2015

@tgoorden I've love to solve this, but I could use your help. Do you have any more feedback on this?

@tgoorden

This comment has been minimized.

Copy link
Author

commented Mar 10, 2015

@FooBarWidget Hi! Sorry for not following up on this earlier.

So here's the situation:

  • Meteor does not support METEOR_SETTINGS when used in development mode (essentially this is whenever you use meteor or meteor run). They also heavily recommend against using development mode in production, so I think the Passenger documentation should reflect that. (Our server admin didn't know, which is why we ended up in trouble.) Since Passenger doesn't pass on --settings to the a Meteor development environment, this way of working is essentially crippled at the moment in Passenger: if you decide to use it, you simply cannot use Meteor settings.
  • When in production (essentially when you use a Meteor bundle, which is basically a NodeJS app), Meteor does support METEOR_SETTINGS. There is however no way to pass --settings to a Meteor bundle.

Summary:

  • Dev mode: --settings supported, METEOR_SETTINGS not supported. Broken in Passenger.
  • Bundle: --settings not supported. METEOR_SETTINGS supported. This works well in Passenger.

So, as it stands, I think it makes sense to focus on production mode and make that as easy to use as possible and recommend it for use. Perhaps you should even considering not offering dev mode or make sure there is a way to pass --settings.

@FooBarWidget

This comment has been minimized.

Copy link
Member

commented Mar 10, 2015

In the next version of Passenger you will be able to set the METEOR_SETTINGS environment variable and have it work for both development and production.

@FooBarWidget

This comment has been minimized.

Copy link
Member

commented Mar 10, 2015

OnixGH pointed out that doing this may not be a good idea because the Meteor authors separated METEOR_SETTINGS and --settings on purpose. Reopening this ticket so I can reinvestigate the issue.

@FooBarWidget FooBarWidget reopened this Mar 10, 2015

@OnixGH OnixGH assigned FooBarWidget and unassigned OnixGH Mar 10, 2015

@FooBarWidget FooBarWidget modified the milestones: 5.0.3, 5.0.4 Mar 11, 2015

@serkandurusoy

This comment has been minimized.

Copy link

commented Apr 12, 2015

This still does not work. Even if --settings does get passed the contents of the env var set on nginx.conf, it is not applied to meteor.

I'm using latest nginx integration mode and meteor (not node bundle).

@FooBarWidget

This comment has been minimized.

Copy link
Member

commented Apr 16, 2015

I'm reopening this issue. It looks like my git push automatically closed this issue -- unintentionally.

@FooBarWidget FooBarWidget reopened this Apr 16, 2015

@FooBarWidget FooBarWidget modified the milestones: 5.0.7, 5.0.5 Apr 16, 2015

@serkandurusoy

This comment has been minimized.

Copy link

commented Apr 16, 2015

Thanks.

In light of the latest changes on meteor, the correct strategy is:

A) If running from meteor build (node) METEOR_SETTINGS should be an environment variable returning valid JSON string

b) If running from meteor run (production) METEOR_SETTINGS is disregarded, instead --settings path-to-settings-file.json should be used

@serkandurusoy

This comment has been minimized.

Copy link

commented Apr 16, 2015

Would it not be cleaner to create two different options, one for meteor_settings and one for --settings where first takes a json string and gets used as an environment variable, and second takes a file path which gets appended to meteor run command.

This way, you won't need to unset anything and you will have conformed to the meteor standard all the while being future proof for when meteor does actually separate the use of those two things as already intended.

@FooBarWidget

This comment has been minimized.

Copy link
Member

commented Apr 17, 2015

Yes that would be a good idea.

@OnixGH Can you implement this? You are supposed to:

  1. Implement configuration options for Apache, Nginx, Standalone (Nginx engine and builtin engine).
  2. Add a serializable field to ApplicationPool2/Options.h.
  3. Modify helper-scripts/node-loader.js and helper-scripts/meteor-loader.rb. In the Node.js loader, set the METEOR_SETTINGS environment variable. In the Meteor loader, pass --settings.

@FooBarWidget FooBarWidget assigned OnixGH and unassigned FooBarWidget Apr 17, 2015

@serkandurusoy

This comment has been minimized.

Copy link

commented Apr 17, 2015

I have a follow up question.

I'm currently using passenger to serve two different apps on the same server. One of them is using METEOR_SETTINGS from the environment.

What happens if I want to serve two different apps which require their own settings?

It's easy with the --settings option since it passes a file which is app specific.

But what about METEOR_SETTINGS? Does it get applied to the whole os/passenger/nginx context or does it get applied to the app context only?

In fact, what's the exact context that that environment variable will have applied to?

@FooBarWidget

This comment has been minimized.

Copy link
Member

commented Apr 17, 2015

We'll allow you to set the Meteor settings on a per-app basis.

@serkandurusoy

This comment has been minimized.

Copy link

commented Apr 17, 2015

👏 👏 👏

@OnixGH OnixGH changed the title Critical: Configuring Meteor through METEOR_SETTINGS is broken/impossible Support for Meteor (development mode) is broken Apr 19, 2015

@OnixGH

This comment has been minimized.

Copy link
Contributor

commented Apr 19, 2015

@FooBarWidget @serkandurusoy
According to the meteor docs, the app is only in production mode when run as a bundle (essentially as a NodeJs app). Using meteor --production is NOT production mode, just a simulation (some minification).

To make this more clear I added a warning when you try to use Passenger in production mode with meteor in development mode (i.e. non-bundled use).

Actually the problem is not development vs. production mode, but bundled vs. non-bundled mode.

Development mode from a bundled meteor app also works with METEOR_SETTINGS. It's just that for anything run by the meteor loader (which happens to always be development mode), METEOR_SETTINGS and --settings, are two different concepts. Passenger should support passing --settings (meant for apps) from the meteor loader without using or touching the METEOR_SETTINGS (meant for meteor).

For bundled mode, i.e. running through node, METEOR_SETTINGS is always the way to configure the app (whether prod or dev). This is fairly confusing when you are switching between bunded and non-bundled. Essentially we could add 2 configuration options for Passenger so it automatically does the right thing if you use those. But that also means we have to deal with existing METEOR_SETTINGS, e.g. what if someone specifies those, but also configures the options in Passenger? Which settings to use, in what case?

I propose to introduce just 1 option to specify a settings file name for Passenger's meteor loader to pass as --settings. This will solve the issue (because METEOR_SETTINGS already work), and we can open a feature thread to see if we want to add another option to work around the confusing difference of METEOR_SETTINGS in bundled/non-bundled (in which we'll also have to solve the question about what to do with existing METEOR_SETTINGS in the env).

@OnixGH OnixGH changed the title Support for Meteor (development mode) is broken Support for Meteor (non-bundled mode) is broken Apr 19, 2015

@serkandurusoy

This comment has been minimized.

Copy link

commented Apr 19, 2015

Thanks for explaining it very thoroughly.

The way I deploy apps depends on what stage of the application's lifecycle I am.

If it is the early stages, getting frequent updates or after some feature/maintenance release that may require further hypercare maintenance, I prefer running the app in development mode with meteor run --production --settings settings.json so that I can point the app root to a git clone of the codebase and just do a git pull to apply patches instantly. This is a huge overall efficiency boost.

And when the app finally is released to enter a normal maintenance cycle, I only then prefer a proper production deployment with meteor build and export METEOR_SETTINGS='{ somekey: "somevalue"}' node main.js .

But the tricky part is, meteor has two different settings in place for a future application where METEOR_SETTINGS will get applied to the meteor command and --settings will get applied to the app itself.

These are currently somewhat interchangeable and confusing.

Deploying with passenger makes this even more confusing because passenger also takes over setting the environment variable and/or --settings flag. This is not passenger's fault per se, but it does add up to the already present confusion.

So I stand by my suggestion to provide two different settings to be passed from the configuration directives and letting meteor itself decide which one (or both) to apply and when. As it stands, meteor just silently ignores unnecessary settings anyway.

This of course can be further documented to ease away the confusion.

@serkandurusoy

This comment has been minimized.

Copy link

commented Apr 19, 2015

Oh and additionaly, as you've put it, if one decides to set METEOR_SETTINGS as a usual environment variable, that sure can take precedence from within the passenger_env_var directive, right?

@OnixGH

This comment has been minimized.

Copy link
Contributor

commented Apr 19, 2015

@serkandurusoy I don't disagree on having two options, but what I was trying to say is that this issue can be fixed with just one option (a way to relay --settings without touching METEOR_SETTINGS). Currently Passenger doesn't do anything with --settings (@FooBarWidget's patch was reverted), so I'd like to finish up.

I see the benefit providing a workaround for Meteor's choice to change the meaning of METEOR_SETTINGS depending on whether you run bundled or non-bundled, but that's not causing or solving this issue and shouldn't be mixed in the commit history. It also requires some more thought, for example if we let the ENV take precedence over the config directives people might get confused if their settings don't get applied when they don't realize they've set the ENV one.

@OnixGH OnixGH changed the title Support for Meteor (non-bundled mode) is broken Support for Meteor app settings (non-bundled mode) is broken Apr 19, 2015

@serkandurusoy

This comment has been minimized.

Copy link

commented Apr 19, 2015

Well you are indeed right about two meteor_settings, one with env one as some other option plus a --settings now actually does sound more confusing.

Then I guess, having proper --settings passing to meteor run along with the passenger_env_var meteor_settings directive would suit whatever meteor need in this interim period until they decide what to do with those for certain.

OnixGH pushed a commit that referenced this issue Apr 21, 2015

OnixGH pushed a commit that referenced this issue Apr 22, 2015

@OnixGH

This comment has been minimized.

Copy link
Contributor

commented Apr 22, 2015

Fixed in stable (coming in 5.0.7)

@OnixGH OnixGH closed this Apr 22, 2015

@serkandurusoy

This comment has been minimized.

Copy link

commented Apr 22, 2015

👏 👏 👏

@FooBarWidget

This comment has been minimized.

Copy link
Member

commented Apr 22, 2015

@OnixGH Don't forget documentation.

OnixGH pushed a commit that referenced this issue Apr 22, 2015

Daniel Knoppel (Phusion)
@OnixGH

This comment has been minimized.

Copy link
Contributor

commented Apr 22, 2015

@FooBarWidget standalone was already documented, but I remembered the apache/nginx right after I merged :)

@serkandurusoy

This comment has been minimized.

Copy link

commented May 8, 2015

@FooBarWidget

This comment has been minimized.

Copy link
Member

commented May 8, 2015

I haven't uploaded the new document to the website yet.

@serkandurusoy

This comment has been minimized.

Copy link

commented May 8, 2015

Oh, ok. Thanks again for the updates.

On May 8, 2015, at 22:10, Hongli Lai notifications@github.com wrote:

I haven't uploaded the new document to the website yet.


Reply to this email directly or view it on GitHub
#1403 (comment).

@iDoMeteor

This comment has been minimized.

Copy link

commented May 11, 2016

I am working on automating deployments of multiple apps on a server and thusly ran into this issue as well.

I have a few thoughts:

  • Just because someone else's process is fubar, doesn't mean that yours has to be. :D
  • If you rely on the JSON string version, then the JSON has to somehow end up in the virtual host config.. talk about super annoying
  • Specifying a file path in the virtual host config.. super easy
  • A file based config can be edited/updated in a pinch and take effect with a simple app process restart, rather than having to copy/paste some huge JSON into the virtual host every time you change a letter
  • Meteor doesn't bundle the settings file though, so that is annoying as well

If it were me, I would make a passenger_meteor_settings option in the virtual host config that can take either a file or JSON and pass it to Meteor in the right way (like exporting METEOR_SETTINGS=`if [ -f file.json ] ; then cat file.json; else echo passenger_meteor_settings; fi', enabling user flexibility and compensating for Meteor's goofiness. It's better to be a smart programmer, than count on only having smart users. :x

For instance, say I have 20 Meteor apps.. some are rocket chat, some are reaction commerce, some are some crap apps. I have a random joe user I don't know running rocket and wants to change his site title.. he can't edit his virtual host config! He can't bundle his settings (I think bundle should include the settings file but whatev) either, but he can edit a file in his home dir.. named say... settings.json. He can then probably restart his own app process and have the change take effect.

However, I am running a crap app but I have SU powers so I edit the virtual host config where I have the settings as JSON because there are only two and they are next to useless anyway so they are hardly ever changed and don't make a huge mess of my vhost. I would probably have rather edited a proper JSON file that could have been linted. ;)

For my apps I am trying to get off Meteor settings anyway, it's such a huge PIA to have hard-coded settings.. it's so 1999. I only use them for changing hard-coded defaults before first run, at which time they load into Mongo so they can be properly administered. BUT I am one of the rare people who use this approach.

@OnixGH OnixGH removed the SupportCentral label Jun 6, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.