Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Versions #6

Open
den-slepnev98 opened this issue Nov 4, 2022 · 7 comments
Open

Versions #6

den-slepnev98 opened this issue Nov 4, 2022 · 7 comments

Comments

@den-slepnev98
Copy link
Collaborator

den-slepnev98 commented Nov 4, 2022

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

@VladyslavSikailo
Copy link

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

@vpodorozh
Copy link

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
Copy link

vpodorozh commented Nov 7, 2022

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

@den-slepnev98
Copy link
Collaborator Author

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

@VladyslavSikailo
Copy link

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

@DavidLambauer
Copy link
Member

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 DavidLambauer reopened this Nov 7, 2022
@DavidLambauer
Copy link
Member

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants