-
-
Notifications
You must be signed in to change notification settings - Fork 64
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 deploy on Heroku (via badge) #231
Conversation
source/dubregistry/mongodb.d
Outdated
import std.process : environment; | ||
string mongodbURI = environment.get("MONGODB_URI", "mongodb://127.0.0.1"); | ||
parseMongoDBUrl(mongoSettings, mongodbURI); | ||
mongoSettings.authMechanism = MongoAuthMechanism.scramSHA1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Default for Mongo3 (released since two years) - many cloud instances don't support none
anymore:
c0b75d2
to
253c48f
Compare
app.json
Outdated
"repository": "https://github.com/heroku/node-js-sample", | ||
"logo": "https://dlang.org/images/dlogo_opengraph.png", | ||
"keywords": ["d", "dlang", "dub", "mirror"] | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Setting up a mirror will be only two clicks away - see e.g. https://devcenter.heroku.com/articles/heroku-button
A mongoDB addon is needed though, but there are many free ones (e.g. mLab).
Think you need to get the github secrets via env to make this work for Heroku. |
README.md
Outdated
@@ -4,3 +4,5 @@ DUB registry | |||
![vibe.d logo](public/images/logo-small.png) Online registry for [dub](https://github.com/dlang/dub/) packages, see <http://code.dlang.org/>. | |||
|
|||
[![Build Status](https://travis-ci.org/dlang/dub-registry.svg)](https://travis-ci.org/dlang/dub-registry) | |||
|
|||
[![Deploy](https://www.herokucdn.com/deploy/button.svg)](https://heroku.com/deploy?template=https://github.com/dlang/dub-registry) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should come with a bit more context, e.g. "You can host your own registry instance for private repos.".
The mirror-mode doesn't need any credentials and this is only intended for the mirror mode. |
That's actually a good use case and once we have figured out the Mongo failure, I will definitely make this as an option. |
e9125e3
to
62e969e
Compare
Good news 🎉
This was due to "vpmreg" being the default database (and thus getting the segfaults). Also for the future it might make sense to show a warning about a non-existent database directly at the
This could be a follow-up to this PR - this one is big enough already. |
6b3bd02
to
ec0da2c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks OKish
Procfile
Outdated
@@ -0,0 +1,3 @@ | |||
default_process_types: | |||
web: ./dub-registry --port $PORT --mirror=https://code.dlang.org --hostname=dub-registry.heroku.com --separate-cron --vv --bind 0.0.0.0 | |||
cron: ./dub-registry --mirror=https://code.dlang.org --run-cron --vv |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not particularly nice as a cron task in a separate process cannot update in-process caches.
Might be problematic during future development, just support a minor Heroku suspend case.
Any other ideas?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not really. Heroku instances fall asleep very soon, so chances are good that the main dyno gets suspended / terminated a lot anyways.
source/dubregistry/mongodb.d
Outdated
MongoClientSettings mongoSettings; | ||
string databaseName = "vpmreg"; | ||
|
||
shared static this() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To avoid another shared static this()
, maybe lazily initialize settings or just parse them everytime connectMongoDB is called (once usually).
README.md
Outdated
Select the "Scheduler" addon and add the following task: | ||
|
||
``` | ||
./dub-registry --mirror=https://code.dlang.org --run-cron --vv |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd say the private registry use-case is more interesting than encouraging dozens of mirrors, in particular with the current inefficient mirror protocol.
ec0da2c
to
976f861
Compare
Thanks for your pull request, @wilzbach! |
Dynos are stopped and restarted on the free version of Heroku quite often anyways and this doesn't affect the default in-process cron job (it's an opt-in CLI flag).
Well, for now, having one free mirror node in a totally different network is quite nice and of course I agree that being able to run a private mirror directly in Heroku would be quite nice, but it's a lot more complicated, e.g.
As a serious company wouldn't host their code with a third-party provider anyways, this is more "fun" use-case, but in any case, this PR should lay the groundworks if this becomes interesting to anyone. Addressed other comments and rebased. |
source/app.d
Outdated
|
||
readOption("mirror", &s_mirror, "URL of a package registry that this instance should mirror (WARNING: will overwrite local database!)"); | ||
readOption("hostname", &hostname, "Domain name of this instance (default: code.dlang.org)"); | ||
readOption("separate-cron", &separateCron, "Use a separate cron job to query for packages."); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could also remove this flag as the dyno must likely gets suspended and one or two additional updates won't be problematic.
The readme still says |
I just found a new use case for this: setting up automatic deployment to Heroku is really easy, so we could do the following setup:
So can we move forward with this? |
Mainly Martin's comment about the mirror vs. private registry still stands. I'd agree that we shouldn't unnecessarily advertise the mirror mode at this point, especially because we don't have means to make this a trusted process (e.g. signing the database snapshot), so encouraging the use of code.dlang.org mirrors that are not controlled by trusted people would not be a good idea. |
OK. Is removing the "Deploy to Heroku" badge from the README enough? I would leave the |
041bc10
to
156d464
Compare
Yes, a staging preview instance for PRs on Heroku would be nice. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Otherwise LGTM.
source/app.d
Outdated
@@ -80,12 +91,20 @@ shared static this() | |||
|
|||
auto router = new URLRouter; | |||
if (s_mirror.length) router.any("*", (req, res) { req.params["mirror"] = s_mirror; }); | |||
router.get("*", (req, res) @trusted { if (!s_checkTask.running) startMonitoring(); }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The check task shouldn't die, so that looks more like a safety guard.
source/app.d
Outdated
|
||
readOption("mirror", &s_mirror, "URL of a package registry that this instance should mirror (WARNING: will overwrite local database!)"); | ||
readOption("hostname", &hostname, "Domain name of this instance (default: code.dlang.org)"); | ||
readOption("separate-cron", &separateCron, "Use a separate cron job to query for packages."); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's remove the separate cron, it seems superfluous, even on Heroku.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Heroku suspends the dynos if they aren't run. How else would we make sure that the DB gets updated at least daily?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok removed.
31334fa
to
afc618b
Compare
@@ -5,6 +5,7 @@ block title | |||
|
|||
block body | |||
- import vibe.textfilter.urlencode; | |||
- import std.algorithm.searching : startsWith; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (!branch_or_version.startsWith("~"))
(below)
afc618b
to
ca6073c
Compare
2b98f05
to
2e3bf97
Compare
So I'm gave this another shot as I want to setup Heroku Review Apps. In short. For each PR, a new temporary dyno will be created and be existent for five days (the limit can be customized).
I tried disabling the userman part, but without luck so far. |
2e3bf97
to
6d4fd7e
Compare
Finally figured it out and it's working -> https://dub-registry-staging.herokuapp.com |
@@ -0,0 +1 @@ | |||
dmd-2.080.1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Required as Vibe.d currently doesn't build with 2.081 (see e.g. vibe-d/vibe.d#2181)
Going to merge this now as this has been open for more than a year, no one seems to be actively objecting this and I want to get started on the Heroku PR preview. |
So I am publicly experimenting with a "Deploy on Heroku badge" to simplify deployment for new mirrors (and do a bit of dog-fooding on the way).
I managed to get the app to start on Heroku, but currently it still fails with:
I will comment on the invidividual fixes needed so far.
This can be reproduced locally, by checking out this branch and then pushing it to Heroku:
(huge thanks to Martin's awesome buildpack)