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

add micro command for composer #31

Merged
merged 13 commits into from
Feb 7, 2017
Merged

add micro command for composer #31

merged 13 commits into from
Feb 7, 2017

Conversation

oqq
Copy link
Member

@oqq oqq commented Jan 25, 2017

This PR provides two new commands for composer install and composer update all php services and will resolve #21.

TODO

  • find docker executable on default paths and request path if not
  • optimize composer command and provide option to add parameters
    To add undefined options it is necessary to provide a service name. Maybe no more composer parameters are required. However they could defined for command if needed.
  • add options for timeouts
  • stop follow-up prozesses if one has failed
  • improve output
  • configure cache and cert paths for composer
    Composers cache directory is shared by default on my machine. Maybe someone other using not macOS should investigate in that. Also I am not able to share my ssh sock with docker containers.
  • only run composer if composer.json exists
  • add install command
  • add update command
  • tests

@coveralls
Copy link

Coverage Status

Coverage decreased (-2.1%) to 32.323% when pulling 91223c0 on oqq:issue_21 into de7c5ae on prooph:master.

@prolic prolic self-requested a review January 26, 2017 11:40
@prolic prolic self-assigned this Jan 26, 2017
'--rm',
'-i',
'--volume',
"$servicePath/$service:/app",
Copy link
Member

Choose a reason for hiding this comment

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

:/app has to be parsed from docker-compose.yml file

Copy link
Member Author

Choose a reason for hiding this comment

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

nope .. /app is always the working directory from composer container.

use Symfony\Component\Console\Output\OutputInterface;
use Symfony\Component\Process\ProcessBuilder;

final class ComposerInstallCommand extends AbstractCommand
{
const DEFAULT_TIMEOUT = 0;
Copy link
Member

Choose a reason for hiding this comment

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

make them private, they have no use for the outside world

Copy link
Member Author

Choose a reason for hiding this comment

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

ha! new php 7.1 stuff.. 👍

@coveralls
Copy link

Coverage Status

Coverage decreased (-4.5%) to 29.947% when pulling fa5c1d7 on oqq:issue_21 into de7c5ae on prooph:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-6.04%) to 28.426% when pulling 0196e21 on oqq:issue_21 into de7c5ae on prooph:master.

@oqq
Copy link
Member Author

oqq commented Jan 27, 2017

@prolic @sandrokeil @codeliner ready for review.
Maybe you have some annotations here. Also some tests on windows are needed.

I will add unit tests tomorrow.

@codeliner
Copy link
Member

@oqq well done!

Question: Do we want to add a micro:composer:require command? I think in this case the service argument is mandatory, but a simple configure(bool $mandatoryServiceArg) can do the trick. Thoughts?

@oqq
Copy link
Member Author

oqq commented Feb 2, 2017

The service selection is also mandatory for install and update. But if it is not provided as argument, you have to choose from a list or the --all option is set.

So it seems there is not difference for a require command and could be done in the same way. 👍

@oqq oqq changed the title [WIP] add micro command for composer add micro command for composer Feb 4, 2017
@oqq
Copy link
Member Author

oqq commented Feb 4, 2017

Could be merged and moved to micro-cli for now.
@codeliner please provide a ticket in micro-cli repository for the require command.

@coveralls
Copy link

Coverage Status

Coverage increased (+1.5%) to 35.935% when pulling db5f7f1 on oqq:issue_21 into de7c5ae on prooph:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+1.5%) to 35.935% when pulling 819abb9 on oqq:issue_21 into de7c5ae on prooph:master.

@prolic
Copy link
Member

prolic commented Feb 4, 2017

thx @oqq I'll check this again tomorrow and will take the required actions.

@prolic prolic merged commit 819abb9 into prooph:master Feb 7, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

add micro command for composer
4 participants