fix(renovate): improve lockfile maintenance and tidy config

[wpessers]

Context
We've been having issues on the lockfile maintenance PRs where the current renovate config causes an infinete loop. This causes a high amount of spam notifications for maintainers, and more importantly consumes a huge amount of unnecessary gh actions minutes.

Solution

• Removed gitignoredAuthors: renovate will now stop pushing after the custom tidy workflow has ran and the commit attributed to otelbot user has been pushed. This is the actual fix, the ignored authors config is what caused renovate to keep force pushing before.
• Replaced gomodTidy by gomodTidyAll in the post update options config. This is a recent new feature in renovate, see: https://docs.renovatebot.com/golang/#monorepo-tidying-for-local-replace-directives and feat(gomod): add gomodTidyAll option for indirect dependency updates in Go monorepos renovatebot/renovate#37138
  The gomodTidy used to tidy only the specific module that received an update and leave all the others where it's used with replace directives stale. e.g.:

  opentelemetry-lambda/collector/go.mod

  Lines 5 to 13 in 3000cda

  	replace github.com/open-telemetry/opentelemetry-lambda/collector/lambdacomponents => ./lambdacomponents
  	replace github.com/open-telemetry/opentelemetry-lambda/collector/lambdalifecycle => ./lambdalifecycle
  	replace github.com/open-telemetry/opentelemetry-lambda/collector/processor/coldstartprocessor => ./processor/coldstartprocessor
  	replace github.com/open-telemetry/opentelemetry-lambda/collector/processor/decoupleprocessor => ./processor/decoupleprocessor
  	replace github.com/open-telemetry/opentelemetry-lambda/collector/receiver/telemetryapireceiver => ./receiver/telemetryapireceiver

  
  I'm thinking this might fix the issue alltogether since we wouldn't even need the github action to run, the go.mod files would all already be clean.
• Only update minor and patch versions in the lockfile maintenance, "updating" majors of indirect deps is a no-op but it has the side-effect of gomodTidy not being run because renovate thinks the branch contains an update major version.

[tylerbenson]

LGTM, but lets get @thompson-tomo to take a look.

[thompson-tomo]

I think removing this is not necessary as it stops the rebase frequency but does so by putting the pr in an edited state blocking updates and then it potentially error if more updates become available. See below for an alternative.

The issue is renovate is seeing that it is out of date as a transitive has changed hence it triggers a force push to update those transitive.

[wpessers]

I really don't think those force pushes are driven by transitive changes. I verified this looking specifically into PR #2542 -> 200 force-pushes contain just 17 distinct trees. 182 consecutive pushes are identical, e.g.1f1ce2e2 and 342e6846 this can be checked via the gh api or using the cli:
gh api repos/open-telemetry/opentelemetry-lambda/git/commits/<commit hash> --jq .tree.sha.

All 200 overwrote a tidy commit by otelbot through the gh workflow we have for that, with un-tidy content. The "edited state" after removal is intentional and I was even more confident in this change when I cross-referenced with the collector and contrib repos, and noticed it's exactly how they run the same tidy workflow (they don't set gitIgnoredAuthors).

[thompson-tomo]

Sure some of them could be the same but some are transitive. If the concern is qty, then what about setting commitLimit to 1 on the lockfile, this way we maintain the ci running. With removing this we block renovate from updating the collector pr's etc.

[thompson-tomo]

I would suggest adding rebasewhen to reduce when the rebase occurs by setting it to behind-base-branch

[wpessers]

rebaseWhen doesn't affect these pushes. What we're seeing here aren't rebases, renovate says so itself in the logs. In the Mend log (https://developer.mend.io/github/open-telemetry/opentelemetry-lambda/-/job/733c6999-a1c0-47f6-8e53-ac6ba49c67ee, log level Debug, branch renovate/lock-file-maintenance-golang) the run logs, in order:

DEBUG: Branch + PR exists but is not scheduled -- will update if necessary (branch="renovate/lock-file-maintenance-golang")
DEBUG: Converting rebaseWhen=auto to rebaseWhen=conflicted because no rule for behind-base-branch applies (branch="renovate/lock-file-maintenance-golang")
DEBUG: Branch already exists (branch="renovate/lock-file-maintenance-golang")
DEBUG: Skipping behind base branch check due to rebaseWhen=conflicted (branch="renovate/lock-file-maintenance-golang")
...
DEBUG: branch.isConflicted(): false (branch="renovate/lock-file-maintenance-golang")
DEBUG: Branch does not need rebasing (branch="renovate/lock-file-maintenance-golang")

And then later on it pushes:

DEBUG: 14 file(s) to commit (branch="renovate/lock-file-maintenance-golang")
...
INFO: Branch updated (branch="renovate/lock-file-maintenance-golang")
{
  "commitSha": "70a79d988848cb9a10b333cf3fcbd5a4e4c0fb23"
}

The pushes we see come from the "content regeneration" path, which branch.isModified() controls, and gitIgnoredAuthors makes that return false (you can see branch.isModified() = false in the same log), which is why the tidy commit gets overwritten. So changing the rebaseWhen config doesn't really impact what is going on here.

[thompson-tomo]

I don't think the cause is the ignoredAuthors, all that is doing is enabling the pr to avoid an edited/error state. It is more likely renovate updates a dependency to a version which is not imported & then go mod tidy removes it. Hence the fix might be to actually get renovate to do the go tidy as if it adds it then go tidys it away then there will be nothing changed & should avoid force push.

[wpessers]

I'm not saying it's the cause. I am saying it could be some sort of safeguard though. And looking at collector and collector-contrib made me wonder if that's the same reason they don't specify the ignored authors either.

[thompson-tomo]

They differ as they don't update indirect. The safeguard of removing the gitIgnoreAuthors is the same as auto adding the stop updating label.

[thompson-tomo]

I am not following

Only update minor and patch versions in the lockfile maintenance, "updating" majors of indirect deps is a no-op but it has the side-effect of gomodTidy not being run because renovate thinks the branch contains an update major version.

Is what you are thinking is that the presence of a major triggers gomodTidy to not be run?

I would be inclined to leave but instead adjust when the rebase is done as well as adjust the postupdateoptions.

[wpessers]

Yeah exactly, I'll refer to renovate's actual code here again for reference just like I did in our Slack conversation: https://github.com/renovatebot/renovate/blob/34c7bcb7a50cafe7c8fd7f94fc679d0325bb3ccc/lib/modules/manager/gomod/artifacts.ts#L273-L278

As you can see later on in that same file, actually each variant of gomodTidy we can run in renovate sits behind that check: https://github.com/renovatebot/renovate/blob/34c7bcb7a50cafe7c8fd7f94fc679d0325bb3ccc/lib/modules/manager/gomod/artifacts.ts#L291-L302

So "keep majors and adjust postupdateoptions" won't make any difference here. There is no possible way to make renovate run any of the gomodTidy* variants as long as it's a branch classified by renovate as one that contains a major update, regardless of whether or not it actually does.

That last part is especially important and it took me a while to understand myself, but in go new major versions are actually new module paths. So for example an offending module I observed in one of the logs was golang-lru, it has a v1 github.com/hashicorp/golang-lru and v2 github.com/hashicorp/golang-lru/v2. The latter is an entirely different module, so there is no valid way for us (or renovate) to bump our existing requires. Changing it to github.com/hashicorp/golang-lru v2.0.0 is just invalid, since v2 versions must live at the /v2 path. Adding a brand new require line for the /v2 path doesn't survive either: nothing in the module graph will be importing that path (the code importing golang-lru lives inside our upstream deps, not in our own repo), so go mod tidy would immediately drop that line as unused. You can also verify that none of our past force pushes (or any commit really) on those renovate lockfile maintenance PRs have ever contained major version changes. For an official source, see: https://go.dev/ref/mod#major-version-suffixes

You can see the tidy skip happen in job https://developer.mend.io/github/open-telemetry/opentelemetry-lambda/-/job/733c6999-a1c0-47f6-8e53-ac6ba49c67ee where you will see lines like:

DEBUG: gomod: major update for github.com/hashicorp/golang-lru (branch="renovate/lock-file-maintenance-golang")

and a similar one for another dep github.com/cenkalti/backoff/v5. These logs are followed later on by a log indicating that the tidy command has been skipped, this happens for each module updated in that branch:

DEBUG: go mod tidy command skipped (branch="renovate/lock-file-maintenance-golang")

All this to simply say, we lose nothing by excluding majors from the indirect rule. As explained above, those indirect major "updates" can't ever land in a go.mod file, so excluding them removes nothing. What we gain here is that the weekly lockfile branch stops being classified as containing major updates, meaning renovate's own gomodTidy* should finally run on these PRs instead of being skipped. The removal of separateMajorMinor: false / separateMultipleMajor: false in this same commit is just cleanup. Those two settings only existed include majors into this branch in a specific way, and with the majors excluded there is no more need for that config.

[thompson-tomo]

Thanks for sharing the code, i think it points out the actual issue:

const mustSkipGoModTidy =
      !config.postUpdateOptions?.includes('gomodUpdateImportPaths') &&
      config.updateType === 'major';


const isGoModTidyAllRequired =
      config.postUpdateOptions?.includes('gomodTidyAll') === true;

const isGoModTidyRequired =
      !mustSkipGoModTidy &&
      (config.postUpdateOptions?.includes('gomodTidy') === true ||
        config.postUpdateOptions?.includes('gomodTidy1.17') === true ||
        config.postUpdateOptions?.includes('gomodTidyE') === true ||
        isGoModTidyAllRequired ||
        (config.updateType === 'major' && isImportPathUpdateRequired));

Based on that code the likely cause of our issue is that mustSkipGoModTidy is returning true hence it is not running. To address this we should set gomodUpdateImportPaths, that way it should run without needing to remove major updates.

So "keep majors and adjust postupdateoptions" won't make any difference here. There is no possible way to make renovate run any of the gomodTidy* variants as long as it's a branch classified by renovate as one that contains a major update, regardless of whether or not it actually does.

Based on the above code, if we set both gomodUpdateImportPaths & a gomodTidy* option it should update major given the usage of ||.

You can also verify that none of our past force pushes (or any commit really) on those renovate lockfile maintenance PRs have ever contained major version changes.

Not the case, If you take a look at https://github.com/open-telemetry/opentelemetry-lambda/pull/2495/changes#diff-84e81f625eb3580b5a4892a5b5ee1d4c441707ce0655c99621c1cbf3f1d80fb4L41 there is 3 examples of major updates being successful through renovate.

[thompson-tomo]

Looking at the logs, https://developer.mend.io/github/open-telemetry/opentelemetry-lambda/-/job/01a00d17-448a-7bb4-94dc-309930aa60b6 it looks like part of the issue is when renovate runs goModTidy it is failing with an error. What i can't tell is why it fails when run by renovate but ok as a ci job.

The only thing it could be is that the workflow is not checking exit Code. We could add the gomodTidyE option to renovate in addition to goModTidyAll and see if that decreases lines changed by the workflow.

[wpessers]

I'm not sure this is related, all those failures were for a completely different renovate PR. They're for the one that updates the collector-other deps.

As stated above also, on the lockfile PRs we know for sure that gomodTidy is being skipped, so this won't affect those lockfile PRs AFAICT.

[thompson-tomo]

Actually it is related as if we remove the ignored authors, pr's such as that will end up in the edited/errored state and blocked from adding additional updates.

[thompson-tomo]

I still get the feeling that removing gitIgnoreAuthors would be masking the issue by putting the pr in an error/edited state which blocks any further changes rather than resolving the root issue.

To that effect, I have #2558 which just adjusts the update options based on the findings here to hopefully tackle the root issue.

[wpessers]

@thompson-tomo I'm not sure I agree, especially given how collector and collector-contrib do not specify any ignored authors and have a similar tidy workflow to the one you introduced in this repo. However I'm okay to keep it as is if there's a proper reason for the other repos not specifying it.