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

Make it easier to get a dev environment up and running #613

Merged
merged 1 commit into from Jul 7, 2015
Merged

Make it easier to get a dev environment up and running #613

merged 1 commit into from Jul 7, 2015

Conversation

lstrojny
Copy link
Contributor

  • Moves the ODM extension to suggest
  • Ignores a few more directories
  • Installs ODM for CI

@lsmith77 lsmith77 added the State: Reviewing This item is being reviewed to determine if it should be accepted. label Jul 1, 2015
@@ -16,6 +16,7 @@ before_script:
- sh -c 'if [ "${TRAVIS_PHP_VERSION}" != "hhvm" ]; then echo "memory_limit = -1" >> ~/.phpenv/versions/$(phpenv version-name)/etc/conf.d/travis.ini; fi;'
- composer require symfony/symfony:${SYMFONY_VERSION} --prefer-source
- composer install --dev --prefer-source
- composer require doctrine/mongodb-odm ~1.0.3
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why not put this into require-dev ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah .. the point is not forcing mongodb odm for local dev?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes

@lsmith77
Copy link
Contributor

lsmith77 commented Jul 2, 2015

what is the problem with the ODM being installed by default? does it cause issues during composer install if you do not have the mongo ext installed? seems like in the tests everything is mocked anyway ..

@makasim
Copy link
Collaborator

makasim commented Jul 2, 2015

Personally I dont like this approach (having partial environments)

@makasim
Copy link
Collaborator

makasim commented Jul 2, 2015

The env has to be the same for all developers and CI.

@lstrojny
Copy link
Contributor Author

lstrojny commented Jul 3, 2015

@lsmith77 you can't run composer install as ODM depends on ext-mongodb which might not be installed. For dev-dependencies I would try to not depend on dependencies, that are hard(er) to install, like PECL extensions. I am probably a good example for the issue, as everytime I want to do something on LiipImagineBundle I have to first edit the composer.json at least temporarily to get a dev env up an running. Not very optimal.

@lsmith77
Copy link
Contributor

lsmith77 commented Jul 3, 2015

I see your point .. or more importantly .. people tend not to be aware of --ignore-platform-reqs option in composer. but I do like the "skip test" but it should be in the extension

@lstrojny
Copy link
Contributor Author

lstrojny commented Jul 3, 2015

@lsmith77 not sure I understand what you mean by "but I do like the 'skip test' but it should be in the extension"

@@ -23,6 +23,10 @@ class GridFSLoaderTest extends \PHPUnit_Framework_TestCase

public function setUp()
{
if (!class_exists('Doctrine\ODM\MongoDB\DocumentRepository')) {
$this->markTestSkipped('Doctrine ODM not installed');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in case our tests require the mongo pecl ext as well to run, we should also check for their existence to determine if to skip

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I thought it would be sufficient to check for ODM, as that’s the one that requires ext/mongodb. My idea was to check for the highest abstraction and not all the way down. But I don’t mind adding it.

@lstrojny
Copy link
Contributor Author

lstrojny commented Jul 7, 2015

OK, basically what’s left is the test for the extension. The rest has been reverted. Should be good to go now.

lsmith77 added a commit that referenced this pull request Jul 7, 2015
Make it easier to get a dev environment up and running
@lsmith77 lsmith77 merged commit 2a58f93 into liip:master Jul 7, 2015
@lsmith77 lsmith77 removed the State: Reviewing This item is being reviewed to determine if it should be accepted. label Jul 7, 2015
@lsmith77
Copy link
Contributor

lsmith77 commented Jul 7, 2015

thx!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants