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

Add extra notes that detail how we use Dependabot #428

Merged
merged 1 commit into from
May 20, 2020

Conversation

benthorner
Copy link
Contributor

@benthorner benthorner commented May 15, 2020

Previously we had regular discussion on GOV.UK about how we can
make Dependabot less burdensome. This adds and consolidates the
notes on Dependabot to record the outcome of those discussions,
so we don't keep having them again, and again, and...

This PR was at one point a discussion area for a potential change for
an account setting. However, we decided to leave it as-is, and instead
add a note to explain why.

At the time of raising this PR, the note about the setting to "Treat PR
approval as a request to merge" is still under discussion. This PR is also
serves as a place to continue that discussion. Some useful facts about
this setting, from previous discussions:

  • Will the commits be signed? Yup, by GitHub. So that shouldn’t break any pipelines that only permit commits from specific entities.
  • Will the commit message be the same? Yup, so this shouldn’t break any pipelines that are parsing the commit messages.

@benthorner benthorner marked this pull request as ready for review May 15, 2020 13:16
@benthorner
Copy link
Contributor Author

Comment from @36degrees

On the Design System team, we’ve restricted it so that only members of the team can push to the master branch. This is so that we can grant write access to all frontend devs at GDS (so that their PR approvals ’count') but two FE devs couldn’t ship a change without our team being involved.

I’d be keen to check that Dependabot doesn’t trip up on this, as in theory it shouldn’t be able to merge to master, but it may try anyway!

Other than that, sounds sane to me.

My understanding is that this setting should be compatible with that way of working: Dependabot has write access to the repo, but not the required admin or maintainer that you've got protecting the master branch. Note: most of our repos don't have this protection.

@heathd
Copy link
Contributor

heathd commented May 18, 2020

it would be good to know specifically what configuration settings you propose to use.

also if you can link to the official documentation of what each option does

Screenshot 2020-05-18 at 10 31 38

the "enable auto-merging to be enabled on projects" sounds as though it can be enabled on a per-project basis?

@benthorner
Copy link
Contributor Author

@heathd this PR makes a few tweaks to the notes to clarify the guidance we already have. The only actual change proposed is to enable: "Treat PR approval as a request to PR".

Although we've checked "Allow auto-merging to be enabled on projects" for individual repos, this conflicts with the guidance about requiring at least one approving review.

I've been unable to find any documentation about this feature, apart from informal discussion that it's equivalent to @dependabot merge, and that it can only be enabled org-wide.

@@ -20,13 +20,18 @@ Update your dependencies frequently rather than in ‘big bang’ batches. This

There are tools which scan GitHub repositories and raise PRs when they find dependency updates. Teams at GDS are using:

* [Dependabot][] - used by GOV.UK, GOV.UK Pay and GovWifi. The GOV.UK team manual contains [guidance on using Dependabot][] and [how the PRs raised should be reviewed][]
* [Dependabot][] - used by GOV.UK, GOV.UK Pay, GovWifi and Digital Marketplace. The GOV.UK docs contain [guidance on using Dependabot][] and [how the PRs raised should be reviewed][]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could add GOV.UK PaaS to this too - dependabot's used for the nodeJS frontend.

source/standards/tracking-dependencies.html.md.erb Outdated Show resolved Hide resolved
source/standards/tracking-dependencies.html.md.erb Outdated Show resolved Hide resolved
@heathd
Copy link
Contributor

heathd commented May 18, 2020

ah ok, so the "Allow auto-merging to be enabled on projects" is a different behaviour to the "Treat PR approval as a request to PR" function?

If I understand correctly:

  1. Allow auto-merging to be enabled on projects would allow a project to enable auto-merging. Auto-merging is where a PR gets merged even if it doesn't have approval from a reviewer?
  2. Treat PR approval as a request to PR means that once a PR has been approved it gets merged

did I understand correctly?

Previously we had regular discussion on GOV.UK about how we can
make Dependabot less burdensome. This adds and consolidates the
notes on Dependabot to record the outcome of those discussions,
so we don't keep having them again, and again, and...
@benthorner benthorner merged commit edc924d into master May 20, 2020
@benthorner benthorner deleted the dependabot-notes branch May 20, 2020 15:00
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

Successfully merging this pull request may close these issues.

3 participants