automate dependency updates

[saihaj]

We don't have any dependency for graphql-js but as we are setting up the site and just in general it is a good idea to keep things up to date. Currently we have to manually upgrade them. We should setup dependabot or renovatebot to manage upgrade and we have dependency PRs auto merge if the CI succeeds.

[IvanGoncharov]

@saihaj Strongly disagree with that.
Updating dependencies requires reviewing changelog for all major/minor releases.
Also frequent updates of individual dependencies create a mess in git history.
Example: https://github.com/ardatan/graphql-tools/commits/master
It's very hard to view git history and see what was changed.

Also call me paranoid but we need to review package-lock.json changes to prevent security problem affected NPM recently (e.g. ua-parser hajack a month ago).
For the same security reasons I'm against merging updates the same day as they published.

Since as you pointed above we don't have any dependencies and our website is static.
Users are not affected if we little bit behind on our dependencies updates.
I prefer if dependencies are updated manually once in couple weeks by human who will check release notes on all direct dependencies.

[saihaj]

It's very hard to view git history and see what was changed.

Well the example you shared yes the git history has many dependency updates but in reality those commits only touch package management files. The git blame for files is what you want to be able to see and that is not impacted at all with this. I mean you doing a dependency or related ones at a time you can easily revert back if something is broken (not that it matters much in our case here).

I prefer if dependencies are updated manually once in couple weeks by human who will check release notes on all direct dependencies.

Well I was initially suggesting CI passes and we merge deps update but to from this we still don't need a human to manually update. We can have the bot open a PR and then someone can review it and once you approve it can auto merge.

Also call me paranoid but we need to review package-lock.json changes to prevent security problem affected NPM recently (e.g. ua-parser hajack a month ago).
For the same security reasons I'm against merging updates the same day as they published.

Its okay if we don't want to merge to avoid day 0 exploits. But we can still utilize tooling to help with doing things which can easily be automated.

couple weeks by human who will check release notes on all direct dependencies.

The PR opened by bot shows everything you need so in review process you can just view there.

Humans can do better things with time. I think it is simpler to just have a tool generate and provide all details that you would manually go hunt for. Then it is as simple as hitting merge button. Which I think helps one save time and we can do more productive things.

[IvanGoncharov]

Well the example you shared yes the git history has many dependency updates but in reality those commits only touch package management files. The git blame for files is what you want to be able to see and that is not impacted at all with this.

@saihaj I'm not speaking about git blame but general history is affected.
When I open history for a repo to see what changes were made since the last time I checked I see a bunch of dependabot stuff. For example, it's impossible to see what was added since last release.

I mean you doing a dependency or related ones at a time you can easily revert back if something is broken (not that it matters much in our case here).

It will make supporting branches that change package-lock.json a nightmare.
For example your new-docs branch.

Well I was initially suggesting CI passes and we merge deps update but to from this we still don't need a human to manually update. We can have the bot open a PR and then someone can review it and once you approve it can auto merge.

Not sure what we saving here tools like npm-check makes updating process easy and intuitive.

Humans can do better things with time. I think it is simpler to just have a tool generate and provide all details that you would manually go hunt for. Then it is as simple as hitting merge button. Which I think helps one save time and we can do more productive things.

Personal opinion: Changes should be made only by humans and validated by tools.
Otherwise, it results in bad code, that nobody reads and "hitting merge button because it's green" mentality.
It should be the opposite, person updating deps should read release notes (for major and minor releases of direct dependencies) makes necessary changes, and only then verify it by running npm test.

[saihaj]

When I open history for a repo to see what changes were made since the last time I checked I see a bunch of dependabot stuff. For example, it's impossible to see what was added since last release.

I mean it could just be possible that all you do between releases is just update deps and ship a new version.

It will make supporting branches that change package-lock.json a nightmare.

packakge-lock.json conflicts are handled by npm

Not sure what we saving here tools like npm-check makes updating process easy and intuitive.

Well you still have to run npm-check something locally, go find release notes for version upgrade and then open a PR. Whereas with this way all you need to do is code review and everything needed which you would manually search around is part of PR.

that nobody reads and "hitting merge button because it's green" mentality.

Well this is saying that anything that is opened and has passing tests can just blindly get merged.

It should be the opposite, person updating deps should read release notes (for major and minor releases of direct dependencies) makes necessary changes, and only then verify it by running npm test.

Well the tests do run when you open a PR.

The point being made here that we manually create PRs how is that any better? Like those chunks are not reviewable? Then should we require all the changelogs for all the deps being updated in PR summary? Cause how does anyone else who is tryna follow understand what was done? How do we ensure the user upgrading deps read all the changelogs? At least this way all the information will be in PR summary and we are certain if someone is hitting that merge button reviewed it.

[yaacovCR]

@IvanGoncharov, I think you are raising some valid points in terms of bot-noise and dependency stability.

Not to plug a particular product, but a casual look at some Renovate options reveals that there are ways to both increase automation and address some of your concerns.

https://docs.renovatebot.com/configuration-options/#groupname

Modifying the example given there only slightly would allow us to group all of our dependencies into one PR like so:

{
  "packageRules": [
    {
      "matchDepTypes": ["devDependencies"],
      "matchUpdateTypes": ["patch", "minor", "major"],
      "groupName": "devDependencies"
    }
  ]
}

https://docs.renovatebot.com/configuration-options/#stabilitydays

Setting this to some lengthy value (at least 3 considering npm's unpublish window!) would help in terms of partially alleviating any malicious update concerns -- maybe 14 is a value that makes sense?

{
  "packageRules": [
    {
      <...>
      "matchDatasources": ["npm"],
      "stabilityDays": 14
    }
  ],
}

It looks like we can reduce the "noise" further by delaying even raising the PR until the stability window has passed:

{
  "packageRules": [
    {
      <...>
      "internalChecksFilter": "strict"
    }
  ],
}

It probably makes sense to separate major and non-major dependency updates into separate PRs, as major updates probably require a bit of extra mental attention.

I think the (so far) unaddressed points you made are the most interesting!

1. Is there a "merge on green bias?" Would maintainers be tempted to do a less thorough review of an automatically generated dependency bump PR , as opposed to one they authored? Probably that depends to some extent on the individual maintainer. I think I would be tempted to merge more quickly with an automatic PR, but I do hope I would resist the temptation. One significant point is that it might be easier to "do the right thing" and review the release notes, if they were collected for me, which is something that Renovate does in many cases quite nicely.
2. What about the humans first vs. tools first debate? That is really the most fascinating question of all here. I think it's mad-crazy that we have reached a day in which everyone agrees that humans and computers should be working together to write/validate/improve computer code. I am not sure whether it has been shown that code quality improves when all code changes are initiated by humans. Maybe it depends on the type of code change and the review process that is set up for machine-initiated changes.

Overall, I think changing dev dependencies on a code base such as this one in which code coverage is pretty high is a pretty low risk activity, and I would suggest that minor/patch updates could be automerged if tests pass, with human review necessary only for major dependency updates, and, of course, if tests fail. I certainly would agree with @saihaj that we are ok to allow a bot to raise PRs without automerging.

I do agree that we should add some config options to address your other concerns.

[yaacovCR]

I did forget to mention that we would need to use a scheduling option as well, maybe updating dependencies every 2 weeks for minor/patch, every 4 weeks for major? In addition to above grouping/stability options.

[yaacovCR]

@IvanGoncharov any follow up thoughts?

[n1ru4l]

On the working group we decided to go with renovate and now need a PR for adding it!