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 Mongodb in installer #1161

Merged
merged 6 commits into from Sep 6, 2015

Conversation

Projects
None yet
5 participants
@michalmuskala
Member

michalmuskala commented Aug 28, 2015

Adds support for:

  • binary_id as default in ecto models in installer
  • binary_id in the model generator
  • default generator options
  • MongoDB adapter in installer
defp get_ecto_adapter("mongodb", app) do
{:mongodb_ecto, Mongo.Ecto,
dev: [database: "#{app}_dev"],
test: [database: "#{app}_test", pool_size: 1],

This comment has been minimized.

@josevalim

josevalim Aug 28, 2015

Member

You don't need to limit the pool size in mongo, do you?

@josevalim

josevalim Aug 28, 2015

Member

You don't need to limit the pool size in mongo, do you?

This comment has been minimized.

@michalmuskala

michalmuskala Aug 28, 2015

Member

I'm not sure we won't get into troubles with resetting the database during tests with concurrent connections.

@michalmuskala

michalmuskala Aug 28, 2015

Member

I'm not sure we won't get into troubles with resetting the database during tests with concurrent connections.

This comment has been minimized.

@josevalim

josevalim Aug 28, 2015

Member

It doesn't hurt to keep it I guess. :)

@josevalim

josevalim Aug 28, 2015

Member

It doesn't hurt to keep it I guess. :)

@josevalim

This comment has been minimized.

Show comment
Hide comment
@josevalim
Member

josevalim commented Aug 28, 2015

:shipit:

@wsmoak

This comment has been minimized.

Show comment
Hide comment
@wsmoak

wsmoak Aug 30, 2015

Contributor

I tested this out and generating a project with --database mongodb worked fine, as did generating a model and then adding/editing/deleting records.

While not strictly related to the installer and this PR, I noticed that the user is instructed to run mix ecto.migrateafter generating a model [1], which I don't think is necessary with mongodb. At least, there are no files in priv/repo/migrations and afaik no structure to maintain.

It doesn't hurt anything to run it (it just says "Already up") but I wonder if the message could be suppressed?

[1] I used mix phoenix.gen.html User users name:string email:string

Contributor

wsmoak commented Aug 30, 2015

I tested this out and generating a project with --database mongodb worked fine, as did generating a model and then adding/editing/deleting records.

While not strictly related to the installer and this PR, I noticed that the user is instructed to run mix ecto.migrateafter generating a model [1], which I don't think is necessary with mongodb. At least, there are no files in priv/repo/migrations and afaik no structure to maintain.

It doesn't hurt anything to run it (it just says "Already up") but I wonder if the message could be suppressed?

[1] I used mix phoenix.gen.html User users name:string email:string

@michalmuskala

This comment has been minimized.

Show comment
Hide comment
@michalmuskala

michalmuskala Aug 30, 2015

Member

This is a great point. I feel like we can go even further and remove the migration message every time the generator is called with the --no-migration option.

Unfortunately this message is today printed in the gen.json or gen.html task, but they don't know anything about migrations. I think the best option would be to move that message to the gen.model task - it knows everything that is needed to decide about printing the message. That means we will need to change the order of messages (routes vs migrations), but I don't think that's a big issue.

Member

michalmuskala commented Aug 30, 2015

This is a great point. I feel like we can go even further and remove the migration message every time the generator is called with the --no-migration option.

Unfortunately this message is today printed in the gen.json or gen.html task, but they don't know anything about migrations. I think the best option would be to move that message to the gen.model task - it knows everything that is needed to decide about printing the message. That means we will need to change the order of messages (routes vs migrations), but I don't think that's a big issue.

@josevalim

This comment has been minimized.

Show comment
Hide comment
@josevalim

josevalim Aug 30, 2015

Member

Unfortunately this message is today printed in the gen.json or gen.html task, but they don't know anything about migrations. I think the best option would be to move that message to the gen.model task - it knows everything that is needed to decide about printing the message. That means we will need to change the order of messages (routes vs migrations), but I don't think that's a big issue.

👍

Member

josevalim commented Aug 30, 2015

Unfortunately this message is today printed in the gen.json or gen.html task, but they don't know anything about migrations. I think the best option would be to move that message to the gen.model task - it knows everything that is needed to decide about printing the message. That means we will need to change the order of messages (routes vs migrations), but I don't think that's a big issue.

👍

@wsmoak

This comment has been minimized.

Show comment
Hide comment
@wsmoak

wsmoak Aug 31, 2015

Contributor

If a migration is needed, then the order of those messages does matter.

For example with Postgres if you generate a model with mix phoenix.gen.html User users name:string and then try mix ecto.migrate without having added the resource in web/router.ex, you'll get a compilation error:

== Compilation error on file web/views/user_view.ex ==
** (CompileError) web/templates/user/edit.html.eex:4: function user_path/3 undefined

So it must be "Add the resource... and then update your repository..." in that case.

ETA: Which should not prevent this PR getting merged -- we're talking about messages printed by the other tasks, not the installer. :)

Contributor

wsmoak commented Aug 31, 2015

If a migration is needed, then the order of those messages does matter.

For example with Postgres if you generate a model with mix phoenix.gen.html User users name:string and then try mix ecto.migrate without having added the resource in web/router.ex, you'll get a compilation error:

== Compilation error on file web/views/user_view.ex ==
** (CompileError) web/templates/user/edit.html.eex:4: function user_path/3 undefined

So it must be "Add the resource... and then update your repository..." in that case.

ETA: Which should not prevent this PR getting merged -- we're talking about messages printed by the other tasks, not the installer. :)

@chrismccord

This comment has been minimized.

Show comment
Hide comment
@chrismccord

chrismccord Sep 1, 2015

Member

I've been out of the loop for a few days. Where do we stand on this? We're prepping a 1.0.1 release, and it would be nice to get this in. What's left?

Member

chrismccord commented Sep 1, 2015

I've been out of the loop for a few days. Where do we stand on this? We're prepping a 1.0.1 release, and it would be nice to get this in. What's left?

@josevalim

This comment has been minimized.

Show comment
Hide comment
@josevalim

josevalim Sep 1, 2015

Member

@chrismccord I don't think we should include new features on 1.0.1. :) Otherwise it may have one tiny bug... which will force us to define 1.0.2 and so on. I would hold this for v1.1.

Member

josevalim commented Sep 1, 2015

@chrismccord I don't think we should include new features on 1.0.1. :) Otherwise it may have one tiny bug... which will force us to define 1.0.2 and so on. I would hold this for v1.1.

@chrismccord

This comment has been minimized.

Show comment
Hide comment
@chrismccord

chrismccord Sep 1, 2015

Member

@josevalim roger that. This border lines on enhancement, but you are right it could introduce new project issues, so we can sit on it

Member

chrismccord commented Sep 1, 2015

@josevalim roger that. This border lines on enhancement, but you are right it could introduce new project issues, so we can sit on it

@chrismccord chrismccord added this to the v1.1 milestone Sep 1, 2015

@michalmuskala

This comment has been minimized.

Show comment
Hide comment
@michalmuskala

michalmuskala Sep 1, 2015

Member

I rebased the PR, and changed the generators not to emit migration instructions when migration is not generated.

This changed a little bit the order of messages (the creating ones and the instructions for user are interleaved). Is this a problem?

The output of gen.json when not generating migrations:

* creating web/controllers/user_controller.ex
* creating web/views/user_view.ex
* creating test/controllers/user_controller_test.exs
* creating web/views/changeset_view.ex

Add the resource to your api scope in web/router.ex:

    resources "/users", UserController

* creating web/models/user.ex
* creating test/models/user_test.exs

The output of gen.html when generating migrations:

* creating web/controllers/user_controller.ex
* creating web/templates/user/edit.html.eex
* creating web/templates/user/form.html.eex
* creating web/templates/user/index.html.eex
* creating web/templates/user/new.html.eex
* creating web/templates/user/show.html.eex
* creating web/views/user_view.ex
* creating test/controllers/user_controller_test.exs

Add the resource to your browser scope in web/router.ex:

    resources "/users", UserController

* creating priv/repo/migrations/20150901192337_create_user.exs
* creating web/models/user.ex
* creating test/models/user_test.exs

Remeber to update your repository by running migrations:

    $ mix ecto.migrate
Member

michalmuskala commented Sep 1, 2015

I rebased the PR, and changed the generators not to emit migration instructions when migration is not generated.

This changed a little bit the order of messages (the creating ones and the instructions for user are interleaved). Is this a problem?

The output of gen.json when not generating migrations:

* creating web/controllers/user_controller.ex
* creating web/views/user_view.ex
* creating test/controllers/user_controller_test.exs
* creating web/views/changeset_view.ex

Add the resource to your api scope in web/router.ex:

    resources "/users", UserController

* creating web/models/user.ex
* creating test/models/user_test.exs

The output of gen.html when generating migrations:

* creating web/controllers/user_controller.ex
* creating web/templates/user/edit.html.eex
* creating web/templates/user/form.html.eex
* creating web/templates/user/index.html.eex
* creating web/templates/user/new.html.eex
* creating web/templates/user/show.html.eex
* creating web/views/user_view.ex
* creating test/controllers/user_controller_test.exs

Add the resource to your browser scope in web/router.ex:

    resources "/users", UserController

* creating priv/repo/migrations/20150901192337_create_user.exs
* creating web/models/user.ex
* creating test/models/user_test.exs

Remeber to update your repository by running migrations:

    $ mix ecto.migrate
@josevalim

This comment has been minimized.

Show comment
Hide comment
@josevalim

josevalim Sep 1, 2015

Member

Can't you print the routes one after you invoke the model?

Member

josevalim commented Sep 1, 2015

Can't you print the routes one after you invoke the model?

@michalmuskala

This comment has been minimized.

Show comment
Hide comment
@michalmuskala

michalmuskala Sep 1, 2015

Member

But we need users to add the routes before running migrations.
Running migrations forces compilation that fails, because there are no route helpers.

Member

michalmuskala commented Sep 1, 2015

But we need users to add the routes before running migrations.
Running migrations forces compilation that fails, because there are no route helpers.

@josevalim

This comment has been minimized.

Show comment
Hide comment
@josevalim
Member

josevalim commented Sep 1, 2015

@Theemuts

This comment has been minimized.

Show comment
Hide comment
@Theemuts

Theemuts Sep 1, 2015

Contributor

It's not possible to let the phoenix.gen.x mix tasks add the new resource to the "/"-scope in router.ex by default, and give users the option to change this behaviour, by passing something like "/api:scope /web/different_router:path" or "--no-router-change" as arguments for example?

Contributor

Theemuts commented Sep 1, 2015

It's not possible to let the phoenix.gen.x mix tasks add the new resource to the "/"-scope in router.ex by default, and give users the option to change this behaviour, by passing something like "/api:scope /web/different_router:path" or "--no-router-change" as arguments for example?

@michalmuskala

This comment has been minimized.

Show comment
Hide comment
@michalmuskala

michalmuskala Sep 2, 2015

Member

@Theemuts I don't think it's possible to insert this into the router. The router is 100% code, and can be quite complex (for example you can have multiple scopes with the same basic route, but different pipelines). Figuring out the right place to put the new route would be horrible.

@chrismccord @josevalim I understand decision to delay new features until a new minor version. Can you give some approximation how long will it take for 1.1? I don't want a concrete date, but rather a time-frame - is it going to be more like a week, a month or a year? Because, if it's going to be a year, it may be sensible to provide some alternate way of creating projects with mongodb.

Member

michalmuskala commented Sep 2, 2015

@Theemuts I don't think it's possible to insert this into the router. The router is 100% code, and can be quite complex (for example you can have multiple scopes with the same basic route, but different pipelines). Figuring out the right place to put the new route would be horrible.

@chrismccord @josevalim I understand decision to delay new features until a new minor version. Can you give some approximation how long will it take for 1.1? I don't want a concrete date, but rather a time-frame - is it going to be more like a week, a month or a year? Because, if it's going to be a year, it may be sensible to provide some alternate way of creating projects with mongodb.

@chrismccord

This comment has been minimized.

Show comment
Hide comment
@chrismccord

chrismccord Sep 2, 2015

Member

is it going to be more like a week, a month or a year?

On the order of months, but much less than a year. I can't say much beyond that unfortunately

Member

chrismccord commented Sep 2, 2015

is it going to be more like a week, a month or a year?

On the order of months, but much less than a year. I can't say much beyond that unfortunately

@wsmoak

This comment has been minimized.

Show comment
Hide comment
@wsmoak

wsmoak Sep 4, 2015

Contributor

If it's going to be that long, I'd love to find an easier way of getting this into the hands of early adopters who aren't quite ready to build it themselves.

I was in a hurry with http://wsmoak.net/2015/08/31/phoenix-ecto-mongodb.html expecting this PR to get merged out from under me at any minute. I'm going to re-work it and see if there's an easier way to get a bleeding-edge installer from Michał’s branch.

Any chance there is already a CI server that the branch could be added to that would make the installer artifact available for download?

Contributor

wsmoak commented Sep 4, 2015

If it's going to be that long, I'd love to find an easier way of getting this into the hands of early adopters who aren't quite ready to build it themselves.

I was in a hurry with http://wsmoak.net/2015/08/31/phoenix-ecto-mongodb.html expecting this PR to get merged out from under me at any minute. I'm going to re-work it and see if there's an easier way to get a bleeding-edge installer from Michał’s branch.

Any chance there is already a CI server that the branch could be added to that would make the installer artifact available for download?

@josevalim

This comment has been minimized.

Show comment
Hide comment
@josevalim

josevalim Sep 4, 2015

Member

@wsmoak we have instructions for building the installer, which is hopefully straight-forward:

https://github.com/phoenixframework/phoenix/tree/master/installer

If it doesn't work, please let us know.

The only issue I can see in using the installer with current Phoenix 1.0 is that ecto.model will always generate a migration. At least this is easy to work around by passing --no-migration until Phoenix 1.1 is out.

Member

josevalim commented Sep 4, 2015

@wsmoak we have instructions for building the installer, which is hopefully straight-forward:

https://github.com/phoenixframework/phoenix/tree/master/installer

If it doesn't work, please let us know.

The only issue I can see in using the installer with current Phoenix 1.0 is that ecto.model will always generate a migration. At least this is easy to work around by passing --no-migration until Phoenix 1.1 is out.

@josevalim

This comment has been minimized.

Show comment
Hide comment
@josevalim

josevalim Sep 6, 2015

Member

@michalmuskala have you fixed the order of messages? And, if so, is this ready to go?

Member

josevalim commented Sep 6, 2015

@michalmuskala have you fixed the order of messages? And, if so, is this ready to go?

@josevalim

This comment has been minimized.

Show comment
Hide comment
@josevalim

josevalim Sep 6, 2015

Member

We have decided to include this in the next release. Thank you everyone!

Member

josevalim commented Sep 6, 2015

We have decided to include this in the next release. Thank you everyone!

josevalim added a commit that referenced this pull request Sep 6, 2015

@josevalim josevalim merged commit 354d0bb into phoenixframework:master Sep 6, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@michalmuskala

This comment has been minimized.

Show comment
Hide comment
@michalmuskala

michalmuskala Sep 6, 2015

Member

Thank you!

Unfortunately, I have not solved the message ordering issue. It's still mixed. I have no idea how to solve this in some approachable way. Two solutions I see is to either store the instructions for user in some agent, and show them all at the end of running generators. I don't like this very much, because it introduces a ton of complexity. The other way is to have additional argument for the gen.model generator, that would make it not show the message but return it instead. But I'm not sure if returning a usable value from mix task is a good practice.

Member

michalmuskala commented Sep 6, 2015

Thank you!

Unfortunately, I have not solved the message ordering issue. It's still mixed. I have no idea how to solve this in some approachable way. Two solutions I see is to either store the instructions for user in some agent, and show them all at the end of running generators. I don't like this very much, because it introduces a ton of complexity. The other way is to have additional argument for the gen.model generator, that would make it not show the message but return it instead. But I'm not sure if returning a usable value from mix task is a good practice.

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