Refactoring/Cleanup/Fixes for Symfony-related Recipes #688

robfrawley · 2016-06-05T17:17:25Z

Q	A
Bug fix?	Yes
New feature?	Yes
BC breaks?	Yes
Deprecations?	No
Fixed tickets	N/A

Removed overridden deploy:assetic:dump task in symfony3 recipe, as it implied this command was no longer valid and should be overridden with deploy:assets:install command. In reality, deploy:assets:install should exist in all symfony versions, as should deploy:assetic:dump (although this is arguable, as deploy:assetic:dump isn't a Symfony command, it is a kriswallsmith/assetic command).
Changed default for deploy:assetic:dump to false (via dump_assets) as this is not a core Symfony package and should not be assumed to be installed. It relies on kriswallsmith/assetic and symfony/assetic-bundle.
Added deploy:assets:install task as this is a symfony-core command that is intended to be called prior to other asset compilation commands (such as deploy:assetic:dump). Its purpose to is to install any bundle assets (within <bundleName>/Resource/public) and put them in web/bundles/<bundleName>.
composer_action created
Fixed SYMFONY_ENV value in env_vars to correctly refer to the configured env value.
Created bin/console to cleanup calls to Symfony console command (there was a ton of duplicated lines prior)
created console_options so the options passed to bin/console reflect what should be passed per the configured environment (for example, the --no-debug options cannot be passed for any environment except for prod and will likely break hard when passed for dev environment)
General cleanup...

robfrawley · 2016-06-05T17:38:22Z

Perhaps of consequence (is this expected behavior?): prior to fixing the travis build by adding an env environment variable (see https://github.com/robfrawley/deployer/blob/master/recipe/common.php#L44), it failed on php 5.5 and 5.6, but passed on 7.0.

5.5: https://travis-ci.org/deployphp/deployer/jobs/135430007
5.6: https://travis-ci.org/deployphp/deployer/jobs/135430009
7.0: https://travis-ci.org/deployphp/deployer/jobs/135430011

It seems odd that it would pass on 7.0 without env defined with the new composer_options closure ( see https://github.com/robfrawley/deployer/blob/master/recipe/common.php#L31 )...is that possibly a bug?

antonmedv · 2016-06-06T07:58:26Z

Nice work!
It's ok on 7.0, but not on 5.5, 5.6?

oanhnn · 2016-06-06T10:10:46Z

recipe/common.php

+ * Composer options
+ */
+env('composer_action', function () {
+    return env('env') === 'prod' ? 'install' : 'update';


I don't think using composer update is good action.
It will update your vendors and composer.lock file when deploying.
If you want update vendors, i think you should run update on your dev server, test all features and make a commit changed composer.lock file.

Agree, install!

6 июня 2016 г., в 17:10, Oanh Nguyen notifications@github.com написал(а):

In recipe/common.php #688 (comment):

@@ -18,13 +18,32 @@
set('clear_paths', []); // Relative path from deploy_path
set('clear_use_sudo', true); // Using sudo in clean commands?

+/**

* Composer options

*/
+env('composer_action', function () {

return env('env') === 'prod' ? 'install' : 'update';
I don't think using composer update is good action.
It will update your vendors and composer.lock file.

—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub https://github.com/deployphp/deployer/pull/688/files/0e75d00bdc2acb9126aad9ad104032562aab76f7#r65866588, or mute the thread https://github.com/notifications/unsubscribe/AAInsFlgoFwW79Zt-6aKiyKXGEZcPhe9ks5qI_IngaJpZM4IuZkt.

It seems needlessly restrictive to hard-code the "install" action. Would you be more content to reverse this logic such that update is only called if the environment is set to dev?

return env('env') === 'dev' ? 'update' : 'install';

The update command is a valid use case for deployer. For example, I am setting up a project to automatically deploy to a staging server after a successful travis build automatically. Regardless, the default is still to use install unless someone changes their environment.

oanhnn · 2016-06-06T10:13:58Z

I don't think using composer update is good action.
It will update your vendors and composer.lock file when deploying.
If you want update vendors, i think you should run update on your dev server, test all features and make a commit changed composer.lock file.

robfrawley · 2016-06-06T16:07:37Z

@oanhnn The intended use-case is for someone who uses deployer to automatically build a staging environment as part of their CI chain. The default action is still to run install --- only when run with a non-production environment would update be used.

robfrawley · 2016-06-06T16:10:02Z

@Elfet The build works on all platforms now that I defined env https://github.com/robfrawley/deployer/blob/master/recipe/common.php#L44

Prior to that, env was empty, and it would fail on 5.5 and 5.6, but passed on 7.0. I mentioned this as it may have exposed an irregularity and/or bug.

As for this pull-request, it is fixed, but you may still want to look into why it broke selectively without the env defined.

oanhnn · 2016-06-07T14:30:09Z

@robfrawley
If you want using composer update, you can make a custom task.

If you want update vendors, i think you should use flow:
Step 1: run composer update on your working machine (may be by hand)
Step 2: test all features (run unit test)
Step 3: make a commit changed composer.lock file.
Step 4: deploy to server

https://blog.engineyard.com/2014/composer-its-all-about-the-lock-file

Other point

server('dev-svr','192.168.1.234')
    ->stage('dev')
    ->env('prod')  // WTF ???
;

I think it makes users puzzled 😱

robfrawley · 2016-06-07T20:06:20Z

recipe/common.php

@@ -17,14 +17,34 @@
 set('http_user', null);
 set('clear_paths', []);         // Relative path from deploy_path
 set('clear_use_sudo', true);    // Using sudo in clean commands?
+set('composer_allow_update', false);


@oanhnn How is this for a compromise?

No update, please. This is not what can be done in recipe by default.

robfrawley · 2016-06-08T01:31:31Z

@oanhnn

I think it makes users puzzled 😱

Are you implying that the value of stage should define the env programatically somehow? That seems outside the scope of this pull-request. I'm just using env as it has been defined, as a separate value to stage.

oanhnn · 2016-06-09T01:13:34Z

Currently, only symfony recipe using env('env'). But after merge this PR, all recipe will have to set env('env'). Variable name makes users puzzled.

robfrawley · 2016-06-09T05:13:41Z

@oanhnn what do you want to call it? env makes sense to me; I don't have any idea what else it would be called... what do you want to call it?

oanhnn · 2016-06-09T06:42:03Z

What do you think about this code?

env('composer_action', 'install');
env('composer_options', 'install --no-dev --verbose --prefer-dist --optimize-autoloader --no-progress --no-interaction');
env('env_vars', ''); // For Composer installation. Like SYMFONY_ENV=prod

robfrawley · 2016-06-10T07:42:07Z

@oanhnn That code doesn't allow for composer options based on the environment. I changed env to build_type ... is that clearer?

robfrawley · 2016-06-10T07:43:49Z

@Elfet Do you have any comments about this pull request and what might need changing for it to be accepted?

antonmedv · 2016-06-10T07:45:21Z

recipe/common.php

+ * Composer options
+ */
+env('composer_action', function () {
+    return get('composer_allow_update') && env('build_env') === 'dev' ? 'update' : 'install';


Only install allowed.

antonmedv · 2016-06-10T07:46:22Z

dump part is ok.
composer update is not.

oanhnn · 2016-06-10T07:55:04Z

@robfrawley

env('composer_action', 'install');
env('composer_options', 'install --no-dev --verbose --prefer-dist --optimize-autoloader --no-progress --no-interaction');
env('env_vars', ''); // For Composer installation. Like SYMFONY_ENV=prod

Why?

No break BC,
Don't need change for old used projects
Allow change action of composer (install or update) very flexible and simple
Don't need env('env') of env('build_env') for all recipe

@Elfet I am agree with you. Users can use composer update by hand or custom task

robfrawley · 2016-06-10T08:00:39Z

@oanhnn and @Elfet Ok - does the current commit satisfy all the requirements, taking both your comments into account?

fix double task removed composer.lock from commit fix for styleci fix for styleci fix for https://travis-ci.org/deployphp/deployer/builds/135430111 added option to allow composer update renamed env to build_env removed composer action logic moved env to only symfony-recepes removed allow update cleanup comment

robfrawley · 2016-06-10T08:24:16Z

recipe/common.php

+ * Composer options
+ */
+env('composer_action', 'install');
+env('composer_options', '{{composer_action}} --verbose --prefer-dist --no-progress --no-interaction --no-dev --optimize-autoloader');


@Elfet Is this acceptable?

This still allow users to use update. And this is ban practice. @oanhnn what you think?

Crosses fingers... I understand your gut-reaction against running update ad-hoc, but it can be useful for automated deployments to dev server. compoaser_options already exists, so it's easy enough to overwrite the whole string, but this just pulls out composer_action. Give me a heads up if this is okay @oanhnn and @Elfet .

@Elfet I think it is ok. Not break BC and users can customize composer_action easily.
I think in Deployer, composer_options will not include composer_action

@oanhnn Is this what you mentioned breaking into a separate PR?

robfrawley · 2016-06-11T03:37:10Z

FYI, I just wanted to confirm what I asserted above about Symfony versions and assetic-bundle, so I wrote the below quickly to do just that. For all currently supported versions of Symfony:

#!/bin/bash

VERSIONS="2.3 2.7 2.8 3.0 3.1"
TMP_DIR="/tmp/symfony-assetic-check"

rm -fr ${TMP_DIR} && mkdir -p ${TMP_DIR}
curl -LsS https://symfony.com/installer -o ${TMP_DIR}/symfony-installer.phar

for version in ${VERSIONS}; do
  echo -n "Symfony v${version}: "
  php ${TMP_DIR}/symfony-installer.phar -q new ${TMP_DIR}/v${version} $version

  grep assetic ${TMP_DIR}/v${version}/composer.lock > /dev/null
  if [[ $? -eq 0 ]]; then
    echo "Assetic Installed"
  else
    echo "Assetic **NOT** Installed"
  fi
done

The script outputs:

Symfony v2.3: Assetic Installed
Symfony v2.7: Assetic Installed
Symfony v2.8: Assetic **NOT** Installed
Symfony v3.0: Assetic **NOT** Installed
Symfony v3.1: Assetic **NOT** Installed

Again, these are just the defaults, and assetic may or may not be installed on any version.

robfrawley · 2016-06-11T09:20:09Z

@Elfet Give me a shout if anything needs to be changed for this to be merge-ready. Otherwise, thanks for such a great tool!

antonmedv · 2016-06-11T09:24:47Z

@robfrawley still need to think and find compromise between no BC break and moving forward. May be this is good for next major v4.

antonmedv · 2016-06-11T09:24:56Z

@oanhnn what you think?

robfrawley · 2016-06-23T05:34:18Z

@Elfet @oanhnn Where you guys at on this? Any thoughts or comments! ;-)

robfrawley · 2016-06-24T16:29:28Z

This needs to also have the "BUG" issue label. (Deployer's Symfony recipes breaks when Symfony version 2.8 is used due to Assetic assumption, and the version 3 recipe calls unrelated commands for "assets:dump" task that it should not.)

robfrawley · 2016-06-24T16:55:28Z

@Elfet Is this pull request particularly controversial for some reason I don't see?

The current implementation is broken on Symfony 2.8 and calls incorrect commands on versions greater than 3. The only minor BC break seems very acceptable when the full scope of what this pull requests fixes is taken into account (and I can't figure a way around that BC break that also addresses the issues here).

BC Breaks

User must set dump_assets to true for Symfony recipe if they want their assets compiled (whereas previously they did not have to). It is significant to note as well, that I would argue this should have been the case anyway as more versions of Symfony (that are currently supported) do not have Assetic by default, and moreover, the versions that did ship with Assetic by default could also not have the package due to the author cleaning up dependencies he may not be using! The assumption for an external command like this should be that it does not exist unless the user specifies that it does, IMHO.

Bug Fixes

Version 2.8 of Symfony now supported (prior it would fail on assetic:dump task due to lack of Assetic in all versions of Symfony >=2.8).
Version 3 of Symfony recipe now correctly calls assets:dump instead of assets:install (which should never have been the case and makes no logical sense).
All versions of Symfony now have task assets:install which is always called (this needs to be called prior to Asset compilation (assetic:dump) and is applicable even if asset compilation is not enabled so that public assets from bundles are properly copied or symlinked into the public web root).
The env_vars variable now properly reflects the configured environment (SYMFONY_ENV={{env}})
Addition of console_options environment variable as anonymous function to generate the options to pass for the Symfony console command depending on the defined env (calling the console command in prod/dev mode with/without --no-debug option fails if the incorrect combination is passed).
The env_vars are now added to all console commands (previously it was only added to the composer commands).

Cleanup/Improvements

Pulled composer action out of composer_options into own environment variable composer_action
Simplification of deploy:vendors anonymous function
The Symfony console command was moved into own environment variable as anonymous function to allow for cleaner task commands that can now be called simply with {{bin/console}} instead of having repetitious statements like trim(get('bin_dir'), '/') . '/console' everywhere.
Usage of sprintf instead of string/variable concatenation
Cleanup of file newlines to standardize it (some tasks had two spaces between their definitions, others had one)

robfrawley · 2016-06-29T13:55:12Z

@Elfet @oanhnn Any time to look this over and provide feedback?!

I'd love to be able to use deployer directly in my projects instead of relying on my fork with these fixes, but it's not usable on some of the Symfony versions I maintain without the bug fixes here. Can we please get this merged? ;-)

antonmedv · 2016-06-29T14:05:52Z

Sorry, for delay. Will look tonight or tomorrow.

29 июня 2016 г., в 20:55, Rob Frawley 2nd notifications@github.com написал(а):

@Elfet https://github.com/elfet @oanhnn https://github.com/oanhnn Any time to look this over and provide feedback?!

I'd love to be able to use deployer directly in my projects instead of relying on my fork with these fixes, but it's not usable on some of the Symfony versions I maintain without the bug fixes here. Can we please get this merged? ;-)

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub #688 (comment), or mute the thread https://github.com/notifications/unsubscribe/AAInsNy6cxZK1Wm5vnbnHcxcifFFG3Npks5qQnlCgaJpZM4IuZkt.

robfrawley · 2016-07-18T20:13:04Z

@Elfet Any progress?

robfrawley · 2016-08-01T18:26:54Z

@oanhnn @Elfet

Do you guys have any intention of merging this? I don't want to spend the time to rebase it if do not.

Please advise and I'd be more than happy to rebase.

oanhnn · 2016-08-02T10:08:52Z

@robfrawley This branch has conflicts that must be resolved. Please resolved (I think you should make new PR fix issue #720 )

antonmedv · 2016-08-04T18:22:50Z

Hi everyone!

Sorry for a BIG delay. After a lot of thinking i'm decided not to merge this PR into 3.x, because of BC. So i'm closing this.

But all that must be moved into 4.x branch, and i believe that can end up with sooner release on Deployer 4.x.

robfrawley · 2016-08-04T18:46:51Z

@Elfet So, after I spend last night and part of the morning working to re-base this and fix the merge conflicts (due to the thumbs up on moving forward from @oanhnn), that work ends up being a waste of my time? This is due to one extremely minor BC break (that only affects the versions of Symfony most people no longer use), and apparently outweighs the 6 bug fixes and 5 improvements...

I'm a bit speechless guys.

antonmedv · 2016-08-04T18:49:12Z

We not going to throw you work away. We a going to merge your changes into 4.x branch.

antonmedv · 2016-08-04T18:50:13Z

I'll open this PR to show what we still really need your work.

antonmedv · 2016-08-04T19:07:33Z

@robfrawley please check 4.x branch.

robfrawley · 2016-08-04T19:45:59Z

👍

antonmedv · 2016-08-04T20:00:35Z

I'm planning to speed up Deployer development and PR integration (with BC) by increasing speed of major releases.

robfrawley force-pushed the master branch 2 times, most recently from 7e8c171 to 15813f6 Compare June 5, 2016 17:19

robfrawley force-pushed the master branch from 5109f73 to 0e75d00 Compare June 5, 2016 17:41

antonmedv added enhancement labels Jun 6, 2016

oanhnn reviewed Jun 6, 2016
View reviewed changes

robfrawley force-pushed the master branch from 0e75d00 to 3a2ad07 Compare June 7, 2016 20:05

robfrawley reviewed Jun 7, 2016
View reviewed changes

robfrawley mentioned this pull request Jun 8, 2016

sf_debug var for flexibility in custom deployments #693

Closed

robfrawley force-pushed the master branch from 3a2ad07 to da9df22 Compare June 10, 2016 07:36

antonmedv reviewed Jun 10, 2016
View reviewed changes

robfrawley force-pushed the master branch from da9df22 to 9e08ae9 Compare June 10, 2016 07:54

robfrawley force-pushed the master branch 2 times, most recently from c74510d to 4ad8524 Compare June 10, 2016 07:59

robfrawley force-pushed the master branch from 4ad8524 to ec3fb29 Compare June 10, 2016 08:02

robfrawley reviewed Jun 10, 2016
View reviewed changes

robfrawley mentioned this pull request Aug 1, 2016

Fix Symfony recipe follow change of asset bundle #721

Closed

robfrawley mentioned this pull request Aug 2, 2016

Fix Symfony recipe follow change of asset bundle #720

Closed

antonmedv closed this Aug 4, 2016

antonmedv reopened this Aug 4, 2016

antonmedv added the bugfix label Aug 4, 2016

antonmedv closed this Aug 4, 2016

jolipixel mentioned this pull request Apr 10, 2017

Can't deploy a Symfony project with dev environment #1161

Closed

Refactoring/Cleanup/Fixes for Symfony-related Recipes #688

Refactoring/Cleanup/Fixes for Symfony-related Recipes #688

Conversation

robfrawley commented Jun 5, 2016 • edited Loading

robfrawley commented Jun 5, 2016 • edited Loading

antonmedv commented Jun 6, 2016

oanhnn Jun 6, 2016 • edited Loading

Choose a reason for hiding this comment

antonmedv Jun 6, 2016

Choose a reason for hiding this comment

robfrawley Jun 6, 2016

Choose a reason for hiding this comment

oanhnn commented Jun 6, 2016

robfrawley commented Jun 6, 2016

robfrawley commented Jun 6, 2016

oanhnn commented Jun 7, 2016 • edited Loading

robfrawley Jun 7, 2016

Choose a reason for hiding this comment

antonmedv Jun 10, 2016

Choose a reason for hiding this comment

robfrawley commented Jun 8, 2016 • edited Loading

oanhnn commented Jun 9, 2016

robfrawley commented Jun 9, 2016 • edited Loading

oanhnn commented Jun 9, 2016

robfrawley commented Jun 10, 2016

robfrawley commented Jun 10, 2016

antonmedv Jun 10, 2016

Choose a reason for hiding this comment

antonmedv commented Jun 10, 2016

oanhnn commented Jun 10, 2016 • edited Loading

robfrawley commented Jun 10, 2016

robfrawley Jun 10, 2016 • edited Loading

Choose a reason for hiding this comment

antonmedv Jun 11, 2016

Choose a reason for hiding this comment

robfrawley Jun 11, 2016

Choose a reason for hiding this comment

oanhnn Jun 12, 2016 • edited Loading

Choose a reason for hiding this comment

robfrawley Jun 13, 2016 • edited Loading

Choose a reason for hiding this comment

robfrawley commented Jun 11, 2016 • edited Loading

robfrawley commented Jun 11, 2016

antonmedv commented Jun 11, 2016

antonmedv commented Jun 11, 2016

robfrawley commented Jun 23, 2016

robfrawley commented Jun 24, 2016

robfrawley commented Jun 24, 2016 • edited Loading

BC Breaks

Bug Fixes

Cleanup/Improvements

robfrawley commented Jun 29, 2016

antonmedv commented Jun 29, 2016

robfrawley commented Jul 18, 2016

robfrawley commented Aug 1, 2016 • edited Loading

oanhnn commented Aug 2, 2016

antonmedv commented Aug 4, 2016

robfrawley commented Aug 4, 2016

antonmedv commented Aug 4, 2016

antonmedv commented Aug 4, 2016

antonmedv commented Aug 4, 2016

robfrawley commented Aug 4, 2016

antonmedv commented Aug 4, 2016

robfrawley commented Jun 5, 2016 •

edited

Loading

robfrawley commented Jun 5, 2016 •

edited

Loading

oanhnn Jun 6, 2016 •

edited

Loading

oanhnn commented Jun 7, 2016 •

edited

Loading

robfrawley commented Jun 8, 2016 •

edited

Loading

robfrawley commented Jun 9, 2016 •

edited

Loading

oanhnn commented Jun 10, 2016 •

edited

Loading

robfrawley Jun 10, 2016 •

edited

Loading

oanhnn Jun 12, 2016 •

edited

Loading

robfrawley Jun 13, 2016 •

edited

Loading

robfrawley commented Jun 11, 2016 •

edited

Loading

robfrawley commented Jun 24, 2016 •

edited

Loading

robfrawley commented Aug 1, 2016 •

edited

Loading