Feature/skip asset update by schmunk42 · Pull Request #278 · fxpio/composer-asset-plugin

schmunk42 · 2017-02-24T03:26:43Z

fixes Add an option to disable asset git/GitHub API requests #251

Testing:

composer global config repositories.foo vcs https://github.com/schmunk42/composer-asset-plugin.git
composer global require fxp/composer-asset-plugin:"dev-feature/skip-asset-update"

Note: github-no-api must be set to true

FXP_SKIP_ASSET_UPDATE="100 minutes" composer update -vv

Result:

🚀 4x faster (eg. with yii2-app-advanced ~10 seconds); after the first run

Currently creates an additional clone in the cache, I am open for discussion about the implementation. But in this way it should not affect existing installations at all.

CC: @cebe @samdark @motin @mikehaertl @tonydspaniard

samdark · 2017-02-24T08:32:46Z

What is the goal of update with does not update?

francoispluchino · 2017-02-24T09:07:45Z

@schmunk42 Thank you for this PR.

Regarding the implementation, it is necessary to use the new configuration system, which allows to configure the plugin globally, by project or by overriding the options with env variables (see Define the config for all projects and Define the config in a environment variable).

As for the option name, since this option is reserved for Github and works with github-no-api, it should be called github-skip-update (and so FXP_ASSET__GITHUB_SKIP_UPDATE for the env variable).

As for location, it is probably best to place it in the GitDriver. this will provide this functionality for all GIT projects, and most importantly, the GithubDriver uses GitDriver when the github-no-api option is enabled. The option name also changes to git-skip-update (FXP_ASSET__GIT_SKIP_UPDATE).

Given that this option can be set globally etc, you must also add a check (false, 0?) to disable it. In this way, we can define globally for all projects a value of 2 days and deactivated for a specific project or a command with the environment variable, only when we want to force the update.

Last point, the unit tests and the doc when it will finalized! :-)

@samdark To update the PHP dependencies, and avoid the asset update search when you only want to add /remove/update PHP dependencies. This can save you a lot of time.

samdark · 2017-02-24T09:35:48Z

This can waste a lot of your time of one of PHP dependencies depends on clientside dependency.

schmunk42 · 2017-02-24T13:38:13Z

@francoispluchino Thanks for the feedback, will update that.

@samdark First of all, if you do not use this option the plugin behaves the same. If you do not have data of a repo, it will download it - always.

If you turn on the option, it will not skip the update of asset itself, but only the synchronization of the repositories already in the cache, if their modification time is within your threshold. Otherwise they'll be synced and the plugin would behave as always.

Sure there might be edge cases, but for day-to-day development I'd say a value of "12 hours" would be totally fine. Checking which updates are necessary could be vastly improved, but that's a separate topic.

I think it's worth a try - my largest testing application with ~350 extensions (>50 bower asset repos) updates in <25 secs.

samdark · 2017-02-24T13:57:22Z

Then I'd make this "no-refetch" thing affect both clientside and PHP packages so there's no partial out of sync situation.

schmunk42 · 2017-02-24T14:08:20Z

Then I'd make this "no-refetch" thing affect both clientside and PHP packages so there's no partial out of sync situation.

Information for PHP packages is usually downloaded via Packagist.
We need to test, if the no-refetch option would affect i.e. custom/private composer git repos.

francoispluchino · 2017-02-24T14:09:05Z

@samdark In Composer, you can only update a specific package using the update command, and thus not the other packages, this proposal makes it possible to have the same behavior on the assets and not to re-download (5 API queries min or sync the git repositories) the list of all tags and branches of all asset respositories.

Currently, if you run the command composer update my/package, all tags and branches of all assets are downloaded, and for a special case, it's cannot necessary. Another example, you try to install a new dependency, but you have an exception, it's not necessary to "sync" all assets repo to try to find the problem.

But I may not have understood your problem.

francoispluchino · 2017-02-24T14:14:05Z

@schmunk42 Normally this only applies only to the Git Vcs Drivers of the assets. Each VCS repository use a new instance of an VCS driver. And for assets, they are specific drivers. So, the php packages are not affected.

francoispluchino · 2017-02-27T15:56:25Z

@schmunk42 Can your pull request be fixed quickly so that we can release the 1.3.0 version? Or would you rather wait for the next version (1.4)?

schmunk42 · 2017-02-27T19:11:11Z

As for location, it is probably best to place it in the GitDriver. this will provide this functionality for all GIT projects, and most importantly, the GithubDriver usesGitDriver when the github-no-api option is enabled. The option name also changes to git-skip-update (FXP_ASSET__GIT_SKIP_UPDATE).

I renamed it, and it's already in ´GitDriver`. Can I check the ENV or do I need to use an internal API?

Given that this option can be set globally etc, you must also add a check (false,0?) to disable it. In this way, we can define globally for all projects a value of 2 days and deactivated for a specific project or a command with the environment variable, only when we want to force the update.

Added check.

Last point, the unit tests and the doc when it will finalized! :-)

Hope you can help me out a bit with the unit tests, docs should be no big deal.

[edit]
About the unit tests ... I'd have started around here testPublicRepositoryWithFilesystemCache.
Would I need to set cache-vcs-dir for the tests? And how to mock the ENV variable?

francoispluchino · 2017-02-28T09:57:25Z

You have an example in the ConfigTest class.

But you should use the Fxp\Composer\AssetPlugin\Config\Config class initialized in the Fxp\Composer\AssetPlugin\FxpAssetPlugin class. And use a mock of Fxp\Composer\AssetPlugin\Config\Config in your tests.

schmunk42 · 2017-02-28T12:01:51Z

@francoispluchino Could you give me a hint with the test in 7a62259

francoispluchino · 2017-02-28T12:35:48Z

I will inject the config into the repoConfig for that you can directly use the Fxp\Composer\AssetPlugin\Config\Config class instead of the FXP_ASSET__GIT_SKIP_UPDATE env variable in the Git driver with $this->reposConfig['asset-config'].

In your test, you will just have to add the mock of the Fxp\Composer\AssetPlugin\Config\Config class with the asset-config key, and mock the Config::get() method with the git-skip-update parameter and return your result.

francoispluchino · 2017-02-28T13:34:09Z

Added by 68fc247.

I was wrong, the Config class is final, so, you can create the new instance of the config in your test.

Given that I had to inject the config into the AssetRepositoryManager class, And that it is injected into all VCS Repositories, it's best to avoid adding another key to repoConfig, You can access the config in the VCS driver like this:

/* @var AssetRepositoryManager $arm */
$arm = $this->repoConfig['asset-repository-manager'];

if (null !== ($skip = $arm->getConfig()->get('git-skip-update'))) {
    // your code ... > strtotime($skip)
}

schmunk42 · 2017-03-07T19:58:40Z

Should be better now, but I still can't cover the following two lines.

I tried to run $gitDriver->getComposerInformation($identifier) twice and also set the cache-vcs-dir.

francoispluchino · 2017-03-07T19:59:03Z

@schmunk42 see my previous comment to use the Fxp Asset Config and not the Composer config.

schmunk42 · 2017-03-07T20:23:47Z

:( I don't get it?!

First thing I noticed is a little typo in the variable name, btw: assert

    /**
     * @var AssetRepositoryManager
     */
    protected $assertRepositoryManager;

Where would I have to add your code-snippet?
It always says unknown key, because I am not in the AbstractAssetsRepositoryTest - I think.

The code is basically running fine, also the mocking of the test config works, it just not reaches the two red lines in the test, I thought because of missing cache files, no?

I'd not mind if you can fix this ;)

btw: I'd mark the feature as experimental in 1.3

francoispluchino · 2017-03-07T20:27:00Z

I do that tomorrow.

samdark · 2017-03-07T20:29:23Z

+    {
+        if (isset($this->config->get('fxp-asset')['git-driver']['skip-update'])) {
+            $skip = $this->config->get('fxp-asset')['git-driver']['skip-update'];
+            if ($skip != 0) {


Could be just if ($skip).

Remove this test after the changes of the if condition.

samdark · 2017-03-07T20:30:06Z

+        if (isset($this->config->get('fxp-asset')['git-driver']['skip-update'])) {
+            $skip = $this->config->get('fxp-asset')['git-driver']['skip-update'];
+            if ($skip != 0) {
+                $localUrl = $this->config->get('cache-vcs-dir') . '/' . preg_replace('{[^a-z0-9.]}i', '-', $this->url) . '/';


How about - and _?

Add the - and _ in your regex expression.

This is taken from https://github.com/composer/composer/blob/master/src/Composer/Repository/Vcs/GitDriver.php#L45

Why should we treat _ differently and why change - at all?

samdark · 2017-03-07T20:30:57Z

+        $io = $this->getMockBuilder('Composer\IO\IOInterface')->getMock();
+
+        $this->config->merge([
+            'config' => [


Should be removed. I just marked this, because I thought this would bring the coverage back to 100%.

francoispluchino · 2017-03-07T21:44:12Z

My snippet must be added in the GitDriver::initialize() method in place of your if condition.

schmunk42 · 2017-03-07T21:50:22Z

I do that tomorrow.

@francoispluchino That would be great!

francoispluchino · 2017-03-07T21:54:00Z

Thanks to clean your code and your commits :-)

schmunk42 · 2017-03-07T22:06:51Z

Thanks to clean your code and your commits :-)

Would be as clean as I can make it right now. I started with this feature before your changes for 1.3, so I had to merge against master two times, I'd like to avoid rebasing on this setup.

francoispluchino

@schmunk42 It only lacks your feature, so the master branch will not change. Your last 4 commits (2589a2c, d68a154, 753eae6, d8ccfe7) are not suitable (see my code reviews).

Regarding the cleaning, you have 3 files, therefore in the worst case, you copy your changes, you rebase, and you edit your Pull Request (without taking into account your last 4 commits above).

If you wish it, remove your tests, I'll take care of it. But clean your PR so I can merge it and make the necessary changes. Thanks!

francoispluchino · 2017-03-08T09:38:05Z

+     */
+    public function initialize()
+    {
+        if (isset($this->config->get('fxp-asset')['git-driver']['skip-update'])) {


You cannot use a hierarchy in the config to use environment variables (FXP_ASSET__GIT_SKIP_UPDATE env variable is mapped with the config.fxp-asset.git-skip-update package configuration).

You use the Composer config and not the Fxp Asset Config (I cannot extend the Composer config).

To access the Fxp asset config from the GitDriver::initialize() method, you must get the instance of AssetRepositoryManager stored in the repoConfig property and call the getConfig() method:

/* @var AssetRepositoryManager $arm */ $arm = $this->repoConfig['asset-repository-manager']; if (null !== ($skip = $arm->getConfig()->get('git-skip-update'))) { // your code ... > strtotime($skip) }

So, replace this if condition by this snippet.

When I dump (print_r) repoConfig in initialize() I get:

Array ( [url] => https://github.com/twbs/bootstrap.git [asset-type] => bower [filename] => bower.json )

There's no asset-repository-manager key?!

Ok I see, the using of the Git driver from the Github driver doesn't include this variable. I add that.

Added by 1ca5831.

This should be fixed. I tested it locally.
I am trying to fix the tests also, but I am currently stuck with this error message Error: Call to undefined method Mock_AssetRepositoryManager_aa5355bb::getConfig()

francoispluchino · 2017-03-08T09:39:03Z

+    {
+        if (isset($this->config->get('fxp-asset')['git-driver']['skip-update'])) {
+            $skip = $this->config->get('fxp-asset')['git-driver']['skip-update'];
+            if ($skip != 0) {


Remove this test after the changes of the if condition.

francoispluchino · 2017-03-08T09:40:22Z

+        if (isset($this->config->get('fxp-asset')['git-driver']['skip-update'])) {
+            $skip = $this->config->get('fxp-asset')['git-driver']['skip-update'];
+            if ($skip != 0) {
+                $localUrl = $this->config->get('cache-vcs-dir') . '/' . preg_replace('{[^a-z0-9.]}i', '-', $this->url) . '/';


Add the - and _ in your regex expression.

francoispluchino · 2017-03-08T15:02:29Z

+     *
+     * @param string $type
+     * @param string $filename
+     */


Add:

$assetConfig = new \Fxp\Composer\AssetPlugin\Config\Config(array( 'git-skip-update' => true, )); /* @var AssetRepositoryManager|\PHPUnit_Framework_MockObject_MockObject $arm */ $arm = $this->getMockBuilder(AssetRepositoryManager::class)->disableOriginalConstructor()->getMock(); $arm->expects($this->any()) ->method('getConfig') ->willReturn($config);

francoispluchino · 2017-03-08T15:02:32Z

+    public function testPublicRepositoryWithSkipUpdate($type, $filename)
+    {
+        $repoUrl = 'https://github.com/fxpio/composer-asset-plugin.git';
+        $identifier = '92bebbfdcde75ef2368317830e54b605bc938123';


Remove the fxp-asset key.

francoispluchino · 2017-03-08T15:02:35Z

+                    ],
+                ],
+                'cache-vcs-dir' => sys_get_temp_dir().'/composer-test-cache-vcs',
+            ],


Add:

'asset-repository-manager' => $arm,

Can I do this in setUp() in the test for all tests?
When using the RepositoryManager in GitDriver, most of the tests fail with...

6) Fxp\Composer\AssetPlugin\Tests\Repository\Vcs\GitDriverTest::testPublicRepositoryWithFilesystemCache with data set #1 ('bower', 'bower.json') Undefined index: asset-repository-manager /app/Repository/Vcs/GitDriver.php:46 /app/Tests/Repository/Vcs/GitDriverTest.php:249 7) Fxp\Composer\AssetPlugin\Tests\Repository\Vcs\GitHubDriverTest::testPrivateRepositoryNoInteraction with data set #0 ('npm', 'package.json') Error: Call to a member function get() on null

But the actual code work, this is just an issue during testing.

Yes of course, you can.

Please have a look at: e676a76

I am getting...

8) Fxp\Composer\AssetPlugin\Tests\Repository\Vcs\GitDriverTest::testPublicRepositoryWithFilesystemCache with data set #1 ('bower', 'bower.json') Trying to configure method "getConfig" which cannot be configured because it does not exist, has not been specified, is final, or is static

I thought we'd avoid that by extending from AssetRepositoryManager - I am getting confused :)

Also the GitHubDriver test now complains about

18) Fxp\Composer\AssetPlugin\Tests\Repository\Vcs\GitHubDriverTest::testNoApi with data set #5 ('bower', 'bower.json', array('0123456789abcdef0123456789abc...234567'), array('master 0123456789abcdef012345...omment')) Error: Call to a member function get() on null

I am unsure if I'd need to mock the config or add actual checks to the GitDriver.php

You must add the asset-repository-manager key in each repoConfig of failed tests.

For GithubDriverTest, you must create a asset config in the setUp() method, and mock the AssetRepositoryManager::getConfig() method returning the instance of asset config.

francoispluchino · 2017-03-08T15:12:13Z

 definitions specify the packages and their main file array as name/value pairs. For an example
 see the [usage informations](index.md#override-the-main-files-for-bower).

+##### config.fxp-asset.git-driver.skip-update (root-only)


Restore the config key config.fxp-asset.git-skip-update, and add an example of package config and explain this feature and its benefits.

{ "config": { "fxp-asset": { "git-skip-update": "3 days" } } }

schmunk42 · 2017-03-08T18:34:52Z

It's green 💚 🍏 📗 💚

schmunk42 · 2017-03-08T20:24:22Z

Hm, so it's red again.
When I run composer update manually, I can see the Git repository files, like repo/config in the corresponding cache dir.

Can I even mock this?

The underlying issue is, that it composer just empties the cache-directory, instead of removing it.

francoispluchino · 2017-03-09T17:10:34Z

Perhaps it is better to dynamically create a local GIT repository for the test, to add a commit, and delete the folder of repository at the end of the test, and not an existing Github repository.

schmunk42 · 2017-03-14T10:51:02Z

Perhaps it is better to dynamically create a local GIT repository for the test, to add a commit, and delete the folder of repository at the end of the test, and not an existing Github repository.

I don't need to add a commit, the issue is that the repo in the tests is completely empty.
Is there a part in the tests where an actual repo is cloned or mocked locally?

schmunk42 · 2017-03-19T21:26:14Z

@francoispluchino Still any help welcome. I tried several times to get coverage back to 100% but without luck.

schmunk42 · 2017-03-23T23:10:24Z

@francoispluchino Please see https://coveralls.io/jobs/24161140/source_files/766890807 - looks to me like my changes are covered 100% now.

francoispluchino · 2017-03-24T08:15:14Z

@schmunk42 Clean your commits and it's ok for me, thanks.

schmunk42 · 2017-03-24T09:24:40Z

Done.

schmunk42 · 2017-03-29T16:31:27Z

@francoispluchino I am sorry to report that this causes some issues in the .lock file, for git-Repos the local paths are getting locked, which is problematic somehow.

I am thinking about a solution. Are there tests for .lock file contents somewhere?

On the other hand, it would be great if you could at least pull this onto a branch in your repo, since it'd greatly simplify testing of the current features.

schmunk42 · 2017-03-29T21:38:02Z

Please see #289

francoispluchino · 2017-03-30T08:06:35Z

@schmunk42 Have I to close the PR #287 and this PR, in favor of the PR #289?

schmunk42 · 2017-03-30T15:12:09Z

Yes. I'll close this, please have a look at the other PR.

schmunk42 mentioned this pull request Feb 24, 2017

Join forces building a PHP base-image for Yii? codemix/yii2-dockerbase#6

Closed

francoispluchino assigned schmunk42 Feb 28, 2017

francoispluchino self-requested a review February 28, 2017 13:40

francoispluchino added the enhancement label Feb 28, 2017

francoispluchino added this to the 1.3.0 milestone Feb 28, 2017

samdark suggested changes Mar 7, 2017

View reviewed changes

francoispluchino requested changes Mar 8, 2017

View reviewed changes

francoispluchino reviewed Mar 8, 2017

View reviewed changes

dxops mentioned this pull request Mar 16, 2017

Could not parse version constraint >=~2: Invalid version string "~2" when installing fullcalendar-scheduler #277

Closed

francoispluchino modified the milestones: 1.4.0, 1.3.0 Mar 17, 2017

schmunk42 force-pushed the feature/skip-asset-update branch from 76eceaf to c913369 Compare March 24, 2017 09:21

added skip asset update, fixes fxpio#251

1da0bc0

schmunk42 force-pushed the feature/skip-asset-update branch from c913369 to 1da0bc0 Compare March 24, 2017 09:25

francoispluchino approved these changes Mar 24, 2017

View reviewed changes

schmunk42 closed this Mar 30, 2017

Conversation

schmunk42 commented Feb 24, 2017

Uh oh!

samdark commented Feb 24, 2017

Uh oh!

francoispluchino commented Feb 24, 2017

Uh oh!

samdark commented Feb 24, 2017

Uh oh!

schmunk42 commented Feb 24, 2017

Uh oh!

samdark commented Feb 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

schmunk42 commented Feb 24, 2017

Uh oh!

francoispluchino commented Feb 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

francoispluchino commented Feb 24, 2017

Uh oh!

francoispluchino commented Feb 27, 2017

Uh oh!

schmunk42 commented Feb 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

francoispluchino commented Feb 28, 2017

Uh oh!

schmunk42 commented Feb 28, 2017

Uh oh!

francoispluchino commented Feb 28, 2017

Uh oh!

francoispluchino commented Feb 28, 2017

Uh oh!

schmunk42 commented Mar 7, 2017

Uh oh!

francoispluchino commented Mar 7, 2017

Uh oh!

schmunk42 commented Mar 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

francoispluchino commented Mar 7, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

francoispluchino commented Mar 7, 2017

Uh oh!

schmunk42 commented Mar 7, 2017

Uh oh!

francoispluchino commented Mar 7, 2017

Uh oh!

schmunk42 commented Mar 7, 2017

Uh oh!

francoispluchino left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

samdark commented Feb 24, 2017 •

edited

Loading

francoispluchino commented Feb 24, 2017 •

edited

Loading

schmunk42 commented Feb 27, 2017 •

edited

Loading

schmunk42 commented Mar 7, 2017 •

edited

Loading

francoispluchino Mar 8, 2017 •

edited

Loading

schmunk42 commented Mar 8, 2017 •

edited

Loading