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 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.

[wpessers]

Don't want to dig up the old thread but FWIW I wasn't talking about "major updates being successful through renovate". My remark was specifically related to one of those pesky force-pushes on an already open renovate PR. I never claimed that renovate didn't update go majors.

[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.

[wpessers]

@thompson-tomo I see you merged main into this, but think we can close this PR now

[thompson-tomo]

@wpessers actually we are still wanting the logic for major updates from this pr, just not the ignore author changes.

[wpessers]

Ah yep good point, forgot that was in here. Removing the authors.

[wpessers]

@thompson-tomo done.