Dependabot integration - Githubissues

joni-jones commented 5 years ago

Investigate a possibility to integrate Depenabot with Magento infrastructure.

Check PR creation to update composer dependencies
POC Dependabot notifications into the slack channel

orlangur commented 5 years ago

@joni-jones I don't think automatic dependency update without real need is a good idea.

Please consider integrating https://insight.symfony.com/what-we-analyse instead which includes scanning for dependencies with vulnerabilities and much more than that.

joni-jones commented 5 years ago

The proposed schema doesn't assume that all dependencies updates should be merged automatically. The PRs will be updated by Dependabot as new versions will be released and can be opened as still as we need. At least we can see all outdated dependencies in the one place and can run CI builds to check if Magento code is compatible with new versions.

Could you provide some explanation of why such mechanism is a bad idea?

Symfony Insight looks like a more "feature complicated" solution and didn't find all feature details which satisfy all described requirements.

orlangur commented 5 years ago

@joni-jones, I'm saying that updating dependencies without a real reason is not a good idea. No matter created manually or automatically. Such operation should be done prior to regression testing phase so that no severe defects introduced due to third-party dependencies.

At least we can see all outdated dependencies in the one place and can run CI builds to check if Magento code is compatible with new versions.

That's exactly what https://insight.symfony.com/what-we-analyse can do, the "Security Projects must not depend on dependencies with known security issues" part. It's not a big deal to create such PR manually then or even produce it with some kind of script whenever such situation occurs.

didn't find all feature details which satisfy all described requirements.

Which requirements are not satisfied by SymfonyInsight?

joni-jones commented 5 years ago

I'm saying that updating dependencies without a real reason is not a good idea

Before each new release, we always face the issue what Magento uses outdated dependencies and it takes a big effort to update dependencies and refactor related code. That's the main idea, do not wait for release packaging and update the dependencies iteratively. 3rd party libraries also have improvements and bug fixes in new releases.

Such operation should be done prior to regression testing phase so that no severe defects introduced due to third-party dependencies.

Why do we need to wait for regression testing as such an update can be as a regular task?

Which requirements are not satisfied by SymfonyInsight?

All requirements are specified in this proposal.

orlangur commented 5 years ago

@joni-jones thanks, this proposal wasn't mentioned here yet.

Before each new release, we always face the issue what Magento uses outdated dependencies and it takes a big effort to update dependencies and refactor related code

This what could be handled on regular basis with Insight as well.

it takes a big effort to update dependencies and refactor related code

Having Insight integrated, we may even put such activities on shoulders of Community. @magento/maintainers-with-private-repository-access @magento/engcom-team-members what do you think?

Provide a tool, which can find the latest version of 3rd party components.

Such tool is named composer update. We cannot rely on Dependabot changing package constraints.

joni-jones commented 5 years ago

Such tool is named composer update. We cannot rely on Dependabot changing package constraints.

Dependabot updates the dependency according to the rule specified in composer.json.

orlangur commented 5 years ago

@joni-jones then it does nothing compared to composer update. Which could be done easily manually as soon as we receive a critical error notification from Insight due to outdated dependency / or we can do such kind of automation easily on our side, not a rocket science.

joni-jones commented 5 years ago

Unfortunately, I didn't find the trial account possibility for SymfonyInsight to try it. So just looked though features list. Seems like a lot of them don't make sense or Magento already has. Like:

Form types should be in Form folders
Symfony staff checking like controllers, bundles, Doctrine, etc.
Static analysis like @TODO comments, methods length, unused params, suppressed warnings, global variables, naming convention, prohibited functions, etc. - Magento already has different static tests for these checks
Some things are controversial like "Interfaces names should end with "Interface"" - there is no a PSR rule for such convention, and PHP has reserved interface word to show that is an interface, also we don't name classes with "Class" word, why the interface should be an exception
Github already has Security Alerts notification mechanism https://help.github.com/en/articles/about-security-alerts-for-vulnerable-dependencies - I don't think that it makes sense to integrate non-free SymfonyInsight to get security updates for 3rd party libraries, which Github already provides

orlangur commented 5 years ago

Some things are controversial like "Interfaces names should end with "Interface""

We actually follow this rule: https://github.com/magento/magento2/blob/2.3-develop/dev/tests/static/framework/Magento/Sniffs/NamingConventions/InterfaceNameSniff.php. Me personally was against it, it is a part of Symfony naming convention.

Github already has Security Alerts notification mechanism

Yep, introduced not so long time ago, but it cannot be used as a check for pull request https://github.blog/2017-11-16-introducing-security-alerts-on-github/

There are 18 critical, 48 major, 31 minor and 15 info checks. You mentioned just 4 of them. This is more a fancy way to produce a good quality code, similar to Scrutinizer or CodeClimate, but also covering the requirement of proposal you trying to fulfill with Dependabot.

Magento already has different static tests for these checks

There is nothing bad with it, it just means we can obtain platinum medal faster.

joni-jones commented 5 years ago

There are 18 critical, 48 major, 31 minor and 15 info checks. You mentioned just 4 of them.

Actually, I've grouped them to don't copy features from the SymfonyInsight list.

it cannot be used as a check for pull request

Not sure if SymfonyInsight can do it, at least, didn't find details about it.

Trying to find a possibility to find a trial account and touch it. Without real examples, it's hard to say does it make sense to pay for some extra features.

orlangur commented 5 years ago

Here is how integration with GitHub may look like: https://insight.symfony.com/docs/github/analyze-a-php-project-on-github.html

Here seems to be placed some kind of trial: https://insight.symfony.com/wizard/project/github Maybe, as a PoC in context of mentioned proposal it makes sense to prepare Magento fork with outdated vulnerable dependency included.

orlangur commented 5 years ago

@joni-jones ?

joni-jones commented 5 years ago

@orlangur https://github.com/magento/architecture/pull/109 was merged. Now we have decided to stay on Dependabot + Slack integration as it's enough for current purposes. Symfony Insight requires access for whole Magento organization and I can't test it on personal account with Magento fork.

orlangur commented 5 years ago

@joni-jones thanks, make sense to me, exactly as discussed in Slack with @buskamuza. I was asking if there is a POC or something as this issue initially states :)

magento / architecture

Dependabot integration #122