Feature/skip asset update

[schmunk42]

• fixes #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]

What is the goal of update with does not update?

[francoispluchino]

@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 calledgithub-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 usesGitDriver 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]

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

[schmunk42]

@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]

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

[schmunk42]

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]

@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]

@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]

@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]

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]

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]

@francoispluchino Could you give me a hint with the test in https://github.com/fxpio/composer-asset-plugin/pull/278/commits/7a622596cf1a4b37ee57f3613a0c8a64e9a04844

[francoispluchino]

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]

Added by 68fc24729c0822423a6bfeb14ec7a33a7e3ae889.

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]

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]

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

[schmunk42]

:( 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]

I do that tomorrow.

[francoispluchino]

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

[schmunk42]

I do that tomorrow.

@francoispluchino That would be great!

[francoispluchino]

Thanks to clean your code and your commits :-)

[schmunk42]

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.

[schmunk42]

It's green 💚 🍏 📗 💚

[schmunk42]

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]

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]

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]

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

[schmunk42]

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

[francoispluchino]

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

[schmunk42]

Done.

[schmunk42]

@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]

Please see https://github.com/fxpio/composer-asset-plugin/pull/289

[francoispluchino]

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

[schmunk42]

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