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

go through dependabot upgrades and test, approve, document, and merge them #11140

Open
jywarren opened this issue Jan 29, 2019 · 1 comment
Open
Labels
dependencies Pull requests that update a dependency file help wanted requires help by anyone willing to contribute multiple-use support supportive-task systems

Comments

@jywarren
Copy link
Member

jywarren commented Jan 29, 2019

Updating this to be a guide to merging dependabot pull requests!

Dependabot automatically opens pull requests (PRs) to upgrade our dependencies -- external libraries we are using. You can see such recently opened PRs at:

My process for this is to look at different aspects of the PR to see if it's good to merge now, of if it needs more testing, or if we need to skip or decline to upgrade.

An example of one we need to skip for now is an upgrade to Bootstrap 5 - #10478 - we aren't ready for such a wide-ranging upgrade, as it'd affect many HTML templates across the project! It's a big endeavor, not just a click-and-merge. Similarly #9581 makes me wary... i'd want to check it manually.

By contrast, #11068 was easy - it's a javascript library, so it doesn't affect server-side site functions, it just formats dates. Easy to merge!

But how exactly did I know the difference? It really incorporates many different signs, so let me list out some I look for and balance in making a decision.

Evaluating a Dependabot pull request 🤖 🔍

  1. does it have a high compatibility score (a measure of whether the same upgrade passed tests on other projects)
  2. did tests pass? If our project has good tests, they'll fail if an update breaks anything.
  3. is the library's function covered by our test suite? Would a breaking change trip a test failure, or could it be silently failing?
    • Look at what system the library is used in. Do we test building assets in Rails and so a breaking change to Sprockets would show up as a failed test? In a JavaScript library, Grunt is required to run tests, so a PR that passes tests with a Grunt upgrade is probably fine! Contrast that with a component in system tests like selenium-webdriver might be expected to break system tests alone -- and it did!
  4. are there any known breaking changes listed (expand the Release notes or Changelog to see) in case it's breaking something we haven't written a test for?
  5. is it a major version increase? (i.e. 4.0.0 -> 5.0.0) or just a bugfix or patch (i.e. 4.0.0 -> 4.0.1)
  6. is it a security risk, that we should try to merge despite higher risk of failure?
  7. did someone else approve it? (haha, but really- esp. someone you know is knowledgeable about the system in question)

What to do 💪 🛠️

  1. Review & comment Once you've reviewed these, it's helpful to note some of them in a comment -- you could even "approve" the pull request and note which factors you looked at. The next person could weigh these with other factors (maybe they know the library or test suite well) and make a final call.
  2. Boot it up and take pics Try starting up the code, either locally or in GitPod, and confirming it boots. If you can look for an area of the app which would be affected by the update, take a screenshot to show it still works, and leave it in a comment!
  3. Highlight logs and warnings If a test did fail, it's really helpful to search for the error in the logs, and quote it in a comment. Maybe there's a clue to what went wrong! Likewise if the Release Notes or Changelog warns of a breaking change or incompatibility, quoting that to be sure people see it is helpful.
  4. Ask Dependabot to rebase If there's a merge conflict, or if a test failure that's due to some unrelated issue comes up, you (or an admin) can comment @dependabot rebase and Dependabot will automatically rebase the PR. Sometimes it'll pass then!
  5. Try getting it to pass Sometimes, small tweaks to the code -- modernizing the way we integrate the library, or updating a second library that's needed -- can get tests to pass. If you're feeling up to it, try forking the PR branch and adding some commits to get it to pass, then link to your PR to show what needs to happen to move forward.

Often a Dependabot PR is a quick merge, but some can linger around for months if we aren't sure what's going wrong, or if we don't have time to work through these steps. Getting some help can move this process forward and we're very grateful for the help!

@grvsachdeva grvsachdeva added the dependencies Pull requests that update a dependency file label Jan 30, 2019
@icarito icarito self-assigned this Feb 13, 2019
@icarito icarito closed this as completed Apr 12, 2019
@icarito icarito reopened this Apr 12, 2019
@jywarren

This comment was marked as outdated.

@jywarren jywarren changed the title (after expanding our tests) go through dependabot upgrades and test, approve, merge them go through dependabot upgrades and test, approve, document, and merge them Jun 4, 2019
@jywarren jywarren pinned this issue Jun 4, 2019
@grvsachdeva grvsachdeva added the help wanted requires help by anyone willing to contribute label Jul 13, 2019
@jywarren jywarren transferred this issue from publiclab/mapknitter May 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file help wanted requires help by anyone willing to contribute multiple-use support supportive-task systems
Projects
None yet
Development

No branches or pull requests

3 participants