Magento-2-Dependency-Checker icon indicating copy to clipboard operation
Magento-2-Dependency-Checker copied to clipboard

Versions

Open den-slepnev98 opened this issue 2 years ago • 7 comments

Me, Dmytro and Vlad have discovered that latest changes to integrity checker works too good. Pipelines has failed - because js and XML dependencies have not been taken into account earlier. We think it would be wise to add possibility for projects to choose which version of integrity checker we can use - like adjusting pipeline config with introducing VERSION variable https://raw.githubusercontent.com/run-as-root/gitlab-pipeline-templates/e5b80914df650b4b76c4e5d050c827c96e3629c5/magento2/integrity-checker.yml

den-slepnev98 avatar Nov 04 '22 12:11 den-slepnev98

I think it's better to introduce a CLI config like --skip-files that will accept patterns like *.js or *.xml

VladyslavSikailo avatar Nov 04 '22 13:11 VladyslavSikailo

I think it's better to introduce a CLI config like --skip-files that will accept patterns like *.js or *.xml

This is a cool addition. However, that is another point of discussion.

Currently, we are facing the problem that the project is not controlling what and how it should be tested - we could get any further issues due to this problem.
Imagine you have added these flags to skip some of the validation, but what if the next release of the integrity checker will introduce additional checks and will not allow skipping them?
and so on...

Don't get me wrong - all dependency issues should be fixed. But I wan it to be more controllable for my projects for example - I want to plan a time to fix dependency troubles instead of running into the failed pipeline without the possibility to revert to previous state and make any deployments.

And BTW, the Current discussion should be moved to pipeline templates projects.

vpodorozh avatar Nov 07 '22 09:11 vpodorozh

I think it's better to introduce a CLI config like --skip-files that will accept patterns like *.js or *.xml

After a small rethinking - I would not allow excluding these dependencies (my thoughts).
JS and XML files are parts of the module -> integrity checker should check dependencies of the module (all parts, PHP,phtml,XML,js, etc...) .

@Dren7755

vpodorozh avatar Nov 07 '22 09:11 vpodorozh

The current ticket is closed. The following issue is open here: https://github.com/run-as-root/gitlab-pipeline-templates/issues/6

den-slepnev98 avatar Nov 07 '22 09:11 den-slepnev98

@vpodorozh what if PHPStan will include new checks in its next release? Would you like to have the possibility to configure using some old versions of PHPStan?

No, you will go to the PHPStan config file and exclude the newly created check. Then you will decide do you need them or not.

VladyslavSikailo avatar Nov 07 '22 10:11 VladyslavSikailo

Throwing in my 2 cents here. As far as I can see, we have 2 issues here.

  1. New Versions of this tool shouldn't end up in a broken pipeline. This is only the case, because the projects you're referring to point to the master. So instead of doing this in your CI Config:
- remote: 'https://raw.githubusercontent.com/run-as-root/gitlab-pipeline-templates/master/magento2/integrity-checker.yml'

You should point to a specific version or hash like so:

  #- remote: 'https://raw.githubusercontent.com/run-as-root/gitlab-pipeline-templates/85d2e510002da88514b90e09dc7df734ff646929/magento2/integrity-checker.yml'

For reference: https://www.aoe.com/techradar/methods-and-patterns/pin-external-dependencies.html

  1. Need of a baseline?!

Similar to PHPStan and other tools, we could introduce a baseline. A baseline is a file that lists all the problems but won't fail the job. It will only show you the issues when you change files with these issues. This comes down to the boy scout principle once again.

I can see value in the baseline, but I think it makes sense to apply it on a module base and not a file base?!

DavidLambauer avatar Nov 07 '22 10:11 DavidLambauer

I'm gonna re-open this issue as long as this discussion goes on :)

DavidLambauer avatar Nov 07 '22 10:11 DavidLambauer