Add Support for Nanobox #1709
Add Support for Nanobox #1709
Conversation
danhunsaker
force-pushed
the
feature/nanobox
branch
from
9b72009 to
0799c31
Compare
| @@ -0,0 +1,16 @@ | |||
| # frozen_string_literal: true | |||
|
|
|||
| namespace :db do | |||
There was a problem hiding this comment.
This is the main change I can see that could affect non-Nanobox users/functionality.
Can you expand on the methodology behind this? Looks like it's overriding the default db:migrate
There was a problem hiding this comment.
It's adding an additional command (db:migrate:setup) to the db:migrate namespace (by using namespace :migrate do instead of task migrate: :environment do), as I understand it. Of course, I might be wrong about that, in which case the command could be renamed to sit outside that namespace. Given that the command itself still works, though, and actively references db:migrate to do it, I'm pretty sure it's not overriding anything.
As to what it does, it simply provides a Rake task that will either setup the DB if it hasn't been already, or migrate the DB otherwise, allowing a single command to be placed in the boxfile.yml as an initialization task. Potentially useful elsewhere, as well, for the same reasons.
There was a problem hiding this comment.
Does that address your concerns?
There was a problem hiding this comment.
Sorry this has been waiting for so long, it's partially because I haven't heard of Nanobox before this PR. Regarding this task.. Is it necessary to check the db existence "before_live"? Could it not be something that runs only once when the nanobox is being created?
There was a problem hiding this comment.
Nanobox doesn't support tasks that only run once on first deploy, unfortunately. I'm also not aware of a mechanism we could use during deploy for determining whether the current deploy is the first (successful) one or not, other than the one given here: have Rake simply try to access the DB. I could try to approach this from a different angle that figures out whether this is an initial deploy or not some other way, but I'm not sure how I'd manage that, and I feel this is the cleanest approach, since Rake tasks have access to the DB with the same settings as the rest of the application, without having to manually set them up externally.
The other alternatives I can think of, at the moment, are:
-
make users do the initial DB setup manually (which is a bit involved, defeats some of the purpose of automated deploy tasks, and may actually work incorrectly given the automated deploy tasks would then run a
db:migratebefore thedb:setuphas a chance to be run) -
rename the task defined here so it doesn't sit inside an existing namespace (which would have no functional difference, but might be less scary to merge?)
Near as I can tell, though, this is the simplest and most effective option for running the db:setup task only when it's actually needed. So unless you really want me to change something, I'd feel safer leaving this as is.
danhunsaker
force-pushed
the
feature/nanobox
branch
from
2a55834 to
221764e
Compare
To be merged after Nanobox Support is merged in the main repo: mastodon/mastodon#1709
danhunsaker
force-pushed
the
feature/nanobox
branch
4 times, most recently
from
ac90620 to
6c19ee4
Compare
|
Doesn't Docker give us reproducible production-like development environments? Nanobox doesn't seem to be a very well-known or widely-used tool. I suggesting trusting admins / developers to choose their own tooling, rather than trying to include every useful tool in this repo. |
|
According to the Docker section of the documentation, using Docker in development is not supported, here. There are a number of elements that Docker just doesn't handle properly on its own. Nanobox does actually use Docker under the hood, but also handles all the song and dance needed to get development features working properly, without the dev having to know a single thing about Docker itself to get it going. As to not including every possible deploy target in the repo, I can't speak to that. I agree that Nanobox isn't terribly widespread, yet, but I could argue that for Scalingo, too (I'd never heard of it before Mastodon), and Nanobox's spread is mostly so minimal due to being so recently out of private beta (I've been using it for about a year, but it's been publicly available only since February, IIRC). That part's not my call, but I thought it might be useful to at least offer the option, since initial configuration is actually a bit involved (as evidenced by the number of files involved, here), but actual use is so simple. |
Gemfile
Outdated
| @@ -3,7 +3,9 @@ | |||
| source 'https://rubygems.org' | |||
| ruby '>= 2.3.0', '< 2.5.0' | |||
|
|
|||
| gem 'pkg-config' | |||
There was a problem hiding this comment.
i'm concerned this will affect other things, can you tell me more about what's happening here/why?
There was a problem hiding this comment.
Sure.
So, it turns out that pkg-config isn't smart enough to properly find the right libraries when used with Nanobox. Since it is only included for building nokogiri, and it's possible to manually provide the correct paths while installing that, leaving it out of Nanobox deploys is preferable (especially since the manual config seems to be flat ignored if pkg-config is present).
This change lets me exclude pkg-config from the install list. My understanding is that, unless explicitly excluded, all package groups are treated as dependencies, so this shouldn't affect other platforms at all. That's part of the reason for using a unique name for the group - to prevent it from affecting any users except those who exclude it intentionally.
I am more than happy to use another approach instead, if anyone is aware of one.
There was a problem hiding this comment.
That all seems very reasonable. This seems to be a minimally invasive changeset as far as non-nanobox installs go, and there are no merge conflicts present, so I am going to approve this.
If you haven't rebased and tested this lately I recommend doing so in case there is anything that you need to change that isn't obvious/a merge conflict. That way if you find anything you can add it to this PR before it's merged (:
There was a problem hiding this comment.
my only specific issue was addressed, and it seems that issues raised by other reviewers were a) also addressed and b) will only affect those using nanobox.
my overall concern is that it seems not a lot of people have experience with nanobox and so this code may be difficult to maintain. ultimately i don't think that's a reason not to approve this changeset though, so, looks good to me!
Gemfile
Outdated
| @@ -3,7 +3,9 @@ | |||
| source 'https://rubygems.org' | |||
| ruby '>= 2.3.0', '< 2.5.0' | |||
|
|
|||
| gem 'pkg-config' | |||
There was a problem hiding this comment.
That all seems very reasonable. This seems to be a minimally invasive changeset as far as non-nanobox installs go, and there are no merge conflicts present, so I am going to approve this.
If you haven't rebased and tested this lately I recommend doing so in case there is anything that you need to change that isn't obvious/a merge conflict. That way if you find anything you can add it to this PR before it's merged (:
danhunsaker
force-pushed
the
feature/nanobox
branch
from
7bb2b49 to
316c2bc
Compare
|
Rebased and tested, but it turns out the Nanobox team needs to build an updated As to maintenance, I'm more than willing to continue to track changes needed to work with Nanobox for the foreseeable future. |
danhunsaker
force-pushed
the
feature/nanobox
branch
from
316c2bc to
037a90c
Compare
|
Got everything resolved, finally. This is good to go! |
danhunsaker
force-pushed
the
feature/nanobox
branch
7 times, most recently
from
01e4c78 to
3d72dde
Compare
- Added support for running Mastodon using Nanobox, both for local development, and for deployment to production - Dev mode tested and is working properly - Deployment is undergoing test as of this writing. If it works, this line will be amended to state success; if not, one or more subsequent commits will provide fixes.
Everything seems to work except routing to the streaming API. Will investigate with the Nanobox staff and make fix commits if needed. Changes made: - Also need `NODE_ENV` in production - Node runs on `:4000` - Use `envsubst` to commit `.env.production` values, since `dotEnv` packages don't always support referencing other variables - Can't precompile assets after `transform` hook, but do this locally so it only has to be done once. - Rails won't create `production.log` on its own, so we do this ourselves. - Some `start` commands run from `/data/` for some reason, so use absolute paths in command arguments
Some minor adjustments to improve functionality: - Fixed routing to `web.stream` instances - Adjust `.env.nanobox` to properly generate a default `SMTP_FROM_ADDRESS` via `envsubst` - Update Nginx configs to properly support the needed HTTP version and headers for proper functionality (the streaming API doesn't work without some of these settings in place)
- Need to leave out `pkg-config` since Nanobox deploys without Ruby's headers - create a gem group to exclude the gem during Nanobox installs, but allow it to remain part of the default set otherwise - Update cron jobs to cover new/updated Rake tasks - Update `.env.nanobox` to include latest defaults and additions
Apparently, nokogumbo (pulled in by sanitize, added with `OEmbed Support for PreviewCard` (mastodon#2337) - 88725d6) tries to install before nokogiri, despite needing nokogiri available to build properly. Instruct it to use the same settings as nokogiri does when building nokogiri directly, instead of via bundler.
The switch to WebPack will rely on the local value of the NODE_ENV evar, so set it to production during asset compilation.
- `pkg-config` Gem no longer causes issues in Nanobox, so revert the Gemfile change which allowed excluding it - Update Nginx configuration files with latest recommendations from production documentation - Rebase on master to Get This Merged™ Everything should be golden!
danhunsaker
force-pushed
the
feature/nanobox
branch
from
3d72dde to
e1d4431
Compare
|
Sorry for making you wait. |
|
No worries! There's a lot going on. |
…upstream Merge upstream changes
Nanobox is a tool for creating consistent (but still disposable) development environments, deploying code to remote servers (with support for AWS [official], Digital Ocean [official], Linode [official], and Proxmox [via third party adapter], with others in development), and for ensuring that the development environment mirrors the production environment as closely as possible, so there are minimal surprises during deployment.
This PR adds support for developing Mastodon in a Nanobox environment, and/or for pushing it into a production Nanobox environment. Development with Nanobox doesn't require deploying via Nanobox, nor does deploying with Nanobox require developing with it. There are advantages to using it for both, though.