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

feat(presets): add config:best-practices preset #21239

Merged
merged 19 commits into from
Jul 2, 2023

Conversation

DuncanCasteleyn
Copy link
Contributor

@DuncanCasteleyn DuncanCasteleyn commented Mar 29, 2023

Changes

A new preset was added which allows you to configure what is considered best practice by the renovate team.

Context

Documentation (please check one with an [x])

  • I have updated the documentation, or
  • No documentation update is required

How I've tested my work (please select one)

I have verified these changes via:

  • Code inspection only, or
  • Newly added/modified unit tests, or
  • No unit tests but ran on a real repository, or
  • Both unit tests + ran on a real repository

@DuncanCasteleyn DuncanCasteleyn changed the title feat(presets): add config:bast-practice preset feat(presets): add config:best-practice preset Mar 29, 2023
Copy link
Collaborator

@HonkingGoose HonkingGoose left a comment

Choose a reason for hiding this comment

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

Some first comments.

lib/config/presets/internal/config.ts Outdated Show resolved Hide resolved
lib/config/presets/internal/config.ts Outdated Show resolved Hide resolved
lib/config/presets/internal/config.ts Outdated Show resolved Hide resolved
@HonkingGoose HonkingGoose added the core:config Related to config capabilities and presets label Mar 31, 2023
Copy link
Member

@viceice viceice left a comment

Choose a reason for hiding this comment

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

🤷‍♂️

Co-authored-by: HonkingGoose <[email protected]>
@DuncanCasteleyn DuncanCasteleyn changed the title feat(presets): add config:best-practice preset feat(presets): add config:best-practices preset Apr 1, 2023
@DuncanCasteleyn DuncanCasteleyn marked this pull request as ready for review April 1, 2023 11:11
@DuncanCasteleyn DuncanCasteleyn linked an issue May 9, 2023 that may be closed by this pull request
10 tasks
lib/config/presets/internal/config.ts Outdated Show resolved Hide resolved
@HonkingGoose
Copy link
Collaborator

We already have two presets that pin dev dependencies.

default:pinDevDependencies 1

Pin dependency versions for devDependencies.

{
  "packageRules": [
    {
      "matchDepTypes": [
        "devDependencies"
      ],
      "rangeStrategy": "pin"
    }
  ]
}

default:pinOnlyDevDependencies 2

Pin dependency versions for devDependencies and retain SemVer ranges for others.

{
  "packageRules": [
    {
      "matchPackagePatterns": [
        "*"
      ],
      "rangeStrategy": "replace"
    },
    {
      "matchDepTypes": [
        "devDependencies"
      ],
      "rangeStrategy": "pin"
    },
    {
      "matchDepTypes": [
        "peerDependencies"
      ],
      "rangeStrategy": "widen"
    }
  ]
}

Footnotes

  1. https://docs.renovatebot.com/presets-default/#pindevdependencies

  2. https://docs.renovatebot.com/presets-default/#pinonlydevdependencies

@rarkins
Copy link
Collaborator

rarkins commented May 9, 2023

Let's use the first

@rarkins
Copy link
Collaborator

rarkins commented May 10, 2023

@JamieMagee @viceice @secustor do you think "dashboard approval for major PRs" should be considered as best practice too? I know we always do it, but is it only possible because we are diligent?

@secustor
Copy link
Collaborator

I don't use it and mark the PRs instead with a major label.
IMO hiding this behind approvals has the down side that users are not notified for updates ( e.g. Github notifications ) anymore.

@HonkingGoose
Copy link
Collaborator

HonkingGoose commented May 10, 2023

@JamieMagee @viceice @secustor do you think "dashboard approval for major PRs" should be considered as best practice too? I know we always do it, but is it only possible because we are diligent?

https://docs.renovatebot.com/key-concepts/dashboard/#require-approval-for-major-updates shows an example on how to do this, but it might be a good idea to add it as a preset?

I like this idea, here's my PR:

@viceice
Copy link
Member

viceice commented May 10, 2023

why happens when the platform doesn't support dashboard?

@HonkingGoose
Copy link
Collaborator

why happens when the platform doesn't support dashboard?

I think you mean what happens here. And my answer is that I don't know what happens then. 😄

@DuncanCasteleyn
Copy link
Contributor Author

DuncanCasteleyn commented May 10, 2023

why happens when the platform doesn't support dashboard?

What happens when you have dependency dashboard disabled and you enable dashboard approval is also something I'm wondering I don't see any checks on that in the code when I checked quickly.

@rarkins
Copy link
Collaborator

rarkins commented May 18, 2023

How about any of these we use in this repo?

  "semanticCommitScope": "deps",
  "automergeType": "pr",
  "prCreation": "immediate",
  "dockerfile": {
    "semanticCommitType": "build"
  },
  "packageRules": [
    {
      "matchPackageNames": ["containerbase/node"],
      "versioning": "node"
    },
    {
      "matchDepTypes": ["dependencies"],
      "semanticCommitType": "build"
    },
    {
      "matchPackageNames": ["semantic-release"],
      "semanticCommitType": "build"
    },
    {
      "matchPackageNames": ["@types/jest"],
      "groupName": "jest monorepo"
    },
    {
      "matchPaths": ["**/__fixtures__/**"],
      "enabled": false
    }
  ]

@HonkingGoose
Copy link
Collaborator

why happens when the platform doesn't support dashboard?

What happens when you have dependency dashboard disabled and you enable dashboard approval is also something I'm wondering I don't see any checks on that in the code when I checked quickly.

@rarkins do you know what happens when a user enables the Dependency Dashboard Approval, when the platform does not support Dependency Dashboard issues? I guess:

  • No Dependency Dashboard issue on the repository
  • Maybe a message in the logs where Renovate complains that it can't create the Dependency Dashboard issue?

@rarkins
Copy link
Collaborator

rarkins commented May 18, 2023

Yes, I think it essentially ruins their workflow then, so we should clarify.. or improve the functionality

@HonkingGoose
Copy link
Collaborator

I created a discussion for us to talk about the "Dependency Dashboard approval"-edgecase:

@HonkingGoose
Copy link
Collaborator

Here's my response to @rarkins question:

How about any of these we use in this repo?

  "semanticCommitScope": "deps",
  "automergeType": "pr",
  "prCreation": "immediate",
  "dockerfile": {
    "semanticCommitType": "build"
  },
  "packageRules": [
    {
      "matchPackageNames": ["containerbase/node"],
      "versioning": "node"
    },
    {
      "matchDepTypes": ["dependencies"],
      "semanticCommitType": "build"
    },
    {
      "matchPackageNames": ["semantic-release"],
      "semanticCommitType": "build"
    },
    {
      "matchPackageNames": ["@types/jest"],
      "groupName": "jest monorepo"
    },
    {
      "matchPaths": ["**/__fixtures__/**"],
      "enabled": false
    }
  ]

Semantic Release stuff

I suspect some things are too specific to Renovate to generalize. We can't assume others will use Semantic Versioning and/or Semantic Release, so things related to that should probably stay out of the preset.

Immediate PR creation and PR automerge

Some people may want automergeType=branch where Renovate creates a branch and then silently merges it into main when the tests pass.

Getting a PR notification for a automerged Renovate PR seems better than hiding the update via branch automerge.

matchPackageNames for Jest types

Do we need this in our own configuration anymore?

     {
       "matchPackageNames": ["@types/jest"],
       "groupName": "jest monorepo"
     },

I ask because there's a group:jestPlusTypes preset 1:

{
  "packageRules": [
    {
      "groupName": "jest monorepo",
      "matchPackageNames": [
        "@types/jest"
      ],
      "matchUpdateTypes": [
        "digest",
        "patch",
        "minor",
        "major"
      ]
    }
  ]
}

config:base extends the group:monorepos and group:recommended presets. Is group:jestPlusTypes in the group: presets we extend from then? Here's the config:base preset:

{
  "extends": [
    ":dependencyDashboard",
    ":semanticPrefixFixDepsChoreOthers",
    ":ignoreModulesAndTests",
    "group:monorepos",
    "group:recommended",
    "replacements:all",
    "workarounds:all"
  ]
}

The fixtures ignore looks nice

     {
       "matchPaths": ["**/__fixtures__/**"],
       "enabled": false
     }

You're onto something here! 😄 We could put this plus other common test directories into a new helper:ignoreCommonTestDirectories preset. We can then put that helper preset into our config:best-practices and/or config:base as needed?

Footnotes

  1. https://docs.renovatebot.com/presets-group/#groupjestplustypes

@HonkingGoose
Copy link
Collaborator

@rarkins can you please read the comments in this PR and tell us what changes you want? This PR is getting stale because it needs maintainer-level input. 😉

@rarkins
Copy link
Collaborator

rarkins commented Jun 28, 2023

I'm OK with it as-is

Copy link
Collaborator

@HonkingGoose HonkingGoose left a comment

Choose a reason for hiding this comment

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

I'm okay with merging this preset. We can always expand the preset later if needed. 😄

@rarkins rarkins requested a review from viceice June 29, 2023 05:50
@rarkins rarkins added this pull request to the merge queue Jul 2, 2023
Merged via the queue into renovatebot:main with commit ccf6704 Jul 2, 2023
@renovate-release
Copy link
Collaborator

🎉 This PR is included in version 35.157.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

configMigration: true,
description: 'Preset with best practices from the Renovate maintainers.',
extends: [
'config:base',

Choose a reason for hiding this comment

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

@viceice This should be 'config:recommended', or? I noticed that in documentation. Not sure what should be updated. Only this file and docs will be generate automatically?

Copy link
Collaborator

@HonkingGoose HonkingGoose Jul 7, 2023

Choose a reason for hiding this comment

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

The config:best-practices preset still uses config:base in the current main branch:

import type { Preset } from '../types';
/* eslint sort-keys: ["error", "asc", {caseSensitive: false, natural: true}] */
export const presets: Record<string, Preset> = {
'best-practices': {
configMigration: true,
description: 'Preset with best practices from the Renovate maintainers.',
extends: [
'config:base',
'docker:pinDigests',
'helpers:pinGitHubActionDigests',
':pinDevDependencies',
],
},

I think this was caused because we merged the config:best-practices preset to main before we merged the config:base -> config:recommended PR. The config:base rename PR didn't fix up this file.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 7, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
core:config Related to config capabilities and presets
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants