composer.installed should not always "return True" when composer.lock is present

[martin-helmich]

Currently, the composer.installed state always returns True with no changes, when the file composer.lock is present ([1], line 114ff.). The basic assumption is probably that the lock file is created by the composer install command, and that its presence is an indicator that composer install does not need to be run again.

This directly contradicts the suggested composer workflow to check the composer.lock into version control. See [2] for reference:

Commit your application's composer.lock (along with composer.json) into version control.

This is important because the install command checks if a lock file is present, and if it is, it downloads the versions specified there (regardless of what composer.json says).

This means that anyone who sets up the project will download the exact same version of the dependencies. Your CI server, production machines, other developers in your team, everything and everyone runs on the same dependencies

So a very typical workflow of deploying a PHP application might consist of cloning an application from git (including a composer.lock file) and then running composer install on this very lock file to install the exact dependency versions from that lock file.

This means that the presence of a composer.lock file is no reason at all to not execute a composer install command (on the contrary, since many deployment workflows rely on this).

Would you be willing to accept a pull request to change this behaviour (if required, triggered by an optional parameter for backwards compatibility)?

[1] https://github.com/saltstack/salt/blob/develop/salt/states/composer.py
[2] https://getcomposer.org/doc/01-basic-usage.md#composer-lock-the-lock-file

[jfindlay]

@martin-helmich, a pull request for this would be most excellent and welcome. I don't think backwards compatibility is an issue since the problematic composer.lock logic only exists on the develop branch.

[martin-helmich]

Thanks for the quick reply, @jfindlay.

since the problematic composer.lock logic only exists on the develop branch.

Great, I hadn't even seen that -- I just stumbled on this by accident while investigating another composer-related issue in 2014.7, but hadn't realized that the composer state in 2014.7 and develop had diverged that far.

In that case, I'd prefer to simply remove the composer.lock check altogether and always run the composer install command (it's the indended usage, ihmo). What's your opinion on this?

[jfindlay]

I don't have any experience using composer. You could ask @JustinCarmony or @ross-p. If neither of them say anything, then I think your proposed solution sounds fine.

[JustinCarmony]

Being a resident PHP/Composer guy who also does Salt, @martin-helmich is correct. I didn't realize we were checking for a composer.lock file and not executing an install command.

I wonder if we should add an option to the salt state to allow people to skip running if it detects it has ran already (since technically it will run each and everytime the state is called). If we do that, we'll need to check something other than the composer.lock, since composer.lock could be present w/o the vendor directory.

[ross-p]

I recently committed a lot of changes, mainly adding composer.update and composer.selfupdate modules, and modifying composer.installed so that it uses the updated stuff. Composer changed its functionality so that nothing is written to stdout now (it all goes to stderr) which had totally broken the old code.

Current states include composer.installed (pre-existing) and composer.update, which I added.

composer.installed seemed to be trying to "only run composer.install module if it hadn't been run before". I kept that functionality in the new develop branch (also in a pending PR to 2014.7).

composer.update now always runs the composer.update module. (It will run composer install if composer.lock does not already exist, or composer update if it does).

Maybe adding a composer.install state which always runs the composer.install module without respect to composer.lock would be a solution?

[ross-p]

Nevermind. I checked out the old code before my change and I see that the composer.installed state was always running composer, so I introduced this bug with my changes.

I will modify the new code so it always runs composer.install by default, and add an optional parameter that makes it only run if there is no vendor directory.

@martin-helmich @JustinCarmony Does that sound like it will fix the problem? Anything better you'd like to see here while I'm in the code?

[JustinCarmony]

That sounds great to me. :) 👍

[ross-p]

The PR is up if anyone cares to look. If you want any more changes made let me know.

[TriAnMan]

Looks that I've found the right way to make composer.installed works only when it must to. Can you look this issue #21929?

[rallytime]

This fix made it in to the latest stable release, 2014.7.5, and looks to be fixed. Can anyone confirm this? Can this be closed?

[ross-p]

This has been fixed since at least 2014.7.2 or .3, I think it's safe to close.

[rallytime]

Awesome! Thanks!