search

aeolusproject / conductor

Aeolus Conductor
http://www.aeolusproject.org/
Apache License 2.0
32 stars 27 forks source link

add foreign keys to the database part one #339

Closed martinpovolny closed 11 years ago

martinpovolny commented 11 years ago

in this branch and pull-request I will add one by one foreign keys to conductor models

once a reviewer is found I would be happy to push part of the work and create a new pull-request for the remaining work

jzigmund commented 11 years ago

contraints created, but it needs to be rebased against master, because of conflict in Gemfile,

ACK.

jzigmund commented 11 years ago

One notice what if you short names of FKs? i.e. "deployments_frontend_realm_id_fk" to "frontend_realm_fk" or something like that, but shorter than in pull request's commits

martinpovolny commented 11 years ago

Let's talk about it tomorrow. I have now some issue to resolve. So please wait with the review until then.

On the index name: it's a best practice to name indexes in such a way that the name includes table name and indexed fields.

But let's talk about it face2face tomorrow.

martinpovolny commented 11 years ago

will rebase this next

martinpovolny commented 11 years ago

I have fixed the habtm problem explained in #346 and fixed a test so that all tests pass now. I have also rebased the patch to master. I have kept the key names as generated by foreigner and believe that the names are according to common conventions (as discussed face2face).

So, please, resume the review so that I can move on to adding more missing constraints and the whole project moves towards the bright future!

martinpovolny commented 11 years ago

I have fixed the patch that accidentally added the method 'image'.

jzigmund commented 11 years ago

After running '$ rails s/c', git status shows Gemfile.lock is modified. Please include Gemfile.lock with foreigner to the pull request.

Otherwise the patchset works as expected, so conditional ACK. After including Gemfile.lock, pull request can be merged.