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

chore(renovate): enable PRs for all non-kubernetes dependencies #4133

Merged
merged 2 commits into from
Jun 20, 2023

Conversation

phisco
Copy link
Contributor

@phisco phisco commented May 31, 2023

Description of your changes

Simplified config to address #4127.

We initially decided to only have security-related updates to dependencies, see #3575 (comment), but that complicates a lot dealing with indirect dependencies (see #4127), I feel like we now have been using renovate enough to switch back to opening PRs for all updates only on master, we can always close the PRs if we don't think we need them, but at least we'll know about the updates and we can always reopen them if needed. We'd still keep the recently introduced rule to disable kubernetes related updates, as those should come in as part of crossplane-runtime version bumps first.

We shouldn't fear regressions being introduced, we have a strong suite of unit tests and work is being done to improve also on the E2Es, #4101.

I have:

  • Read and followed Crossplane's contribution process.
  • Run make reviewable to ensure this PR is ready for review.
  • Added backport release-x.y labels to auto-backport this PR if necessary.

How has this code been tested

Deployed in my fork, see phisco#137 for the PRs that would be created at the moment.

Copy link
Contributor

@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 see you're using the helpers:pinGitHubActionDigests preset, and use description fields, like I suggested. 😄

There's a typo, and the proper noun Docker should be capitalized.

.github/renovate.json5 Outdated Show resolved Hide resolved
.github/renovate.json5 Outdated Show resolved Hide resolved
.github/renovate.json5 Outdated Show resolved Hide resolved
Co-authored-by: HonkingGoose <[email protected]>
Signed-off-by: Philippe Scorsolini <[email protected]>
@phisco phisco marked this pull request as ready for review June 8, 2023 13:30
@phisco phisco requested review from a team as code owners June 8, 2023 13:30
@negz
Copy link
Member

negz commented Jun 14, 2023

@phisco If it were possible for Renovate to catch when we needed to update an indirect dependency due to a known vulnerability I would still prefer that. Given that doesn't seem possible, I mostly feel okay with this change.

I'm not really worried about regressions, or increased maintenance work due to extra Renovate PRs to review. My only concern is (or was) that semi-automatically updating all of our dependencies whenever a new version was available could negate some of the benefits of Go's minimal version selection. I re-read https://research.swtch.com/vgo-principles today to refresh my memory on what those benefits are, and to see whether my concern was founded.

I believe the short version is that given a hypothetical diamond dependency:

  graph TD;
      crossplane-->controller-runtime;
      crossplane-->client-go;
      controller-runtime-->yaml;
      client-go-->yaml;
Loading

Where:

  • The latest available yaml == 1.5
  • controller-runtime requires yaml <= 1.0
  • client-go requires yaml <= 1.1

Most package managers would pick (and lock) yaml == 1.5. Go would instead pick yaml == 1.1 - i.e. the oldest possible version that satisfies all constraints. It chooses to do this because:

  • It enables reproducible builds without using a lock file (which is hard to make work for libraries).
  • It lets you avoid dealing with incompatible diamond dependencies until you actually need to. (e.g. To address a vulnerability, or get access to new functionality.)

I don't think semi-automatically updating the minimum versions we depend on hurts reproducible builds in any way. The nature of minimal version selection ensures any particular commit of Crossplane (or something depending on a particular commit of crossplane-runtime) will be reproducible, dependencies wise.

It does seem like semi-automatically updating the minimum versions we depend on will mean we'll need to address incompatible diamond dependencies before we might otherwise need to. Not really a big deal, given we can just close the PR if there's no obvious benefit to bumping and we don't feel like dealing with it. One consideration though is that for crossplane-runtime (i.e. a library) we'll effectively be signing up anyone using it to resolve any conflicts that result from a bunch of potentially unnecessary dependency updates any time they update to a new version of crossplane-runtime.

@HonkingGoose
Copy link
Contributor

The Renovate docs have an article about pinning JavaScript dependencies 1. Here's the table of contents:

  • What is dependency pinning?
  • Why use ranges?
  • Why pin dependencies?
  • Reducing the "noise" of dependency updates
  • Pinning dependencies and lockfiles
  • What about indirect/sub-dependencies?
  • So what's best?
  • References

There are probably big differences between the Go-way and the JavaScript-way, but I think/hope the article can still help you decide what to do with your Go dependencies. 😉

Footnotes

  1. Renovate docs, Should you Pin your JavaScript Dependencies?

@phisco
Copy link
Contributor Author

phisco commented Jun 19, 2023

@negz I'm not sure I've got your point, but in you example, if yaml is an indirect dependency, you won't get a PR to bump it per se, it will only be bumped to the minimum version selected by the MVS algorithm.

The only difference with the current behaviour would be that now we bump only direct dependencies and only in case of vulnerabilities, and there is no way to get those updates on indirect ones and no way to transitively trigger bumps on direct dependencies importing affected indirect ones, while enabling all updates to direct dependencies we'll hopefully get less vulnerabilities on indirect dependencies. This should happen because the indirect dependency will be bumped as soon as the minimum version required by all our up-to-date direct dependencies will be higher than the affected one, which if our direct dependencies are healthy and maintained, should happen pretty fast.

@HonkingGoose
Copy link
Contributor

HonkingGoose commented Jun 19, 2023

The Renovate docs mention indirect updates in the gomod manager section: 1

Indirect updates are disabled by default. To enable them, add a package rule such as:

{
  "packageRules": [
    {
      "matchManagers": ["gomod"],
      "matchDepTypes": ["indirect"],
      "enabled": true
    }
  ]
}

I also found related stuff:

Footnotes

  1. Renovate docs, gomod manager, Post-update options

@phisco
Copy link
Contributor Author

phisco commented Jun 19, 2023

@HonkingGoose yes, but as per the discussion here, it's not possible to enable indirect update only on security vulnerabilities

@negz
Copy link
Member

negz commented Jun 20, 2023

I'm not sure I've got your point

@phisco My understanding is that with this change in place, Renovate will bump all Go dependencies whenever a new version of a dependency is available. So assuming we merge all the Renovate PRs, the versions listed in go.mod will always be the latest possible versions. Is that right?

The big comment I wrote earlier was really just me thinking out loud to try understand how that relates to "idiomatic" Go version selection. My conclusion was they seem pretty compatible. Go obviously still uses MVS at build-time, it's just that we'll always be telling it the minimum allowed version is the latest available version, even if there were no known security issues with an older version, and even if there's no features we or any other dependency needs in the latest version.

The only downside I could see is that proactively updating all our dependencies means we also have to proactively resolve new dependency conflicts if and when they appear. The good thing is we'd be doing this in order to get a Renovate PR to merge, not in order to unbreak our build.

So TL;DR assuming all this works like I think it works I feel good about merging. Should I go ahead and do that?

@phisco
Copy link
Contributor Author

phisco commented Jun 20, 2023

So assuming we merge all the Renovate PRs, the versions listed in go.mod will always be the latest possible versions. Is that right?

@negz this will be true only for direct dependencies, indirect ones will only be updated if the MVS algorithm decides it's time to do so because all direct dependencies require an higher minimum version.

And yes, please, you can merge this 🙏

@negz negz merged commit 5570fb5 into crossplane:master Jun 20, 2023
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