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: update renovate processes: more auto-merge, more often #36

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

tommartensen
Copy link
Contributor

@tommartensen tommartensen commented Feb 6, 2025

@tommartensen tommartensen self-assigned this Feb 6, 2025
@tommartensen tommartensen requested a review from a team as a code owner February 6, 2025 09:06
@tommartensen tommartensen changed the title chore: update renovate config: more auto-merge, more often chore: update renovate processes: more auto-merge, more often Feb 6, 2025
@tommartensen tommartensen enabled auto-merge (squash) February 6, 2025 09:12
],
// Tell Renovate not to update PRs when outside of schedule.
// Tell Renovate to update PRs even when outside of schedule.
"updateNotScheduled": false,
Copy link
Contributor

@msugakov msugakov Feb 6, 2025

Choose a reason for hiding this comment

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

We know that Konflux instance of Renovate was configured to work once in 4 hours and at any time schedule would allow Renovate to check CI status, rebase branches if needed and auto-merge.

However, it may be not the only way for auto-merging to happen at any time.
First, there is automergeSchedule setting which is at any time by default. Meaning when the change qualifies for automerging, Renovate will do that even if outside of the schedule.
Second, there's updateNotScheduled which, after we set it to true, will allow rebases and and bringing new updates to the change branches which are currently open.

In such a combination, schedule will limit times when new branches or PRs (depending on the automergeType) are created. This may not make a lot of visible differences for "automergeType": "branch" but will be helpful in case of "automergeType": "pr" because will avoid opening many PRs during the day.

I'm so much concerned about this because I get quite a bit of Renovate emails already (just as you are) and it may become impractical after we onboard more ACS versions and start getting even more PRs. Moreover, if PR is not actionable by us (yet), it would be better to not receive any emails at all.

@@ -45,4 +45,6 @@
"enabledManagers": [
"tekton",
],
labels: ["auto-merge"],
Copy link
Contributor

Choose a reason for hiding this comment

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

[nit] Yes, json5 allows unquoted names but this still looks a bit unusual compared to other attributes. If we keep this attribute, let's please make it look consistent with others:

Suggested change
labels: ["auto-merge"],
"labels": ["auto-merge"],

@@ -45,4 +45,6 @@
"enabledManagers": [
"tekton",
],
labels: ["auto-merge"],
"platformAutomerge": true
Copy link
Contributor

Choose a reason for hiding this comment

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

[nit] An advantage of json5 is that it allows trailing comma. The one helps have less changed lines in diffs. If we keep this attribute, please add one

Suggested change
"platformAutomerge": true
"platformAutomerge": true,

@@ -34,7 +34,7 @@
"schedule": [
// For some reason, Konflux config defines custom schedule on each type of dependency manager and that takes
// precedence over the global/default schedule. We want our own schedule and hence need to make this override.
"after 3am and before 7am",
"at any time",
],
"automerge": true,
// PRs can't be actually automerged because we require approval from CODEOWNERS which Renovate can't bypass,
Copy link
Contributor

Choose a reason for hiding this comment

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

I feel this PR in its current form is a bit like trying to sit on two chairs.

There should be either

"automergeType": "branch",
// no labels
// no platformAutomerge

or

"automergeType": "pr",
"labels": ["auto-merge"],
"platformAutomerge": true,

Meaning we should:

  1. Either let branch automerge to happen without PRs, and if Renovate gets to open a PR, the one is most likely for human attention because CI failed.
  2. Or make Renovate always open PRs and those that pass CI get auto-merged.

The current settings would work as if we give two chances for auto-merging: first as branch, next as PR. When CI fails at both stages, the PR would be up for our attention.

I'm not fond of Renovate opening PRs because I think we will be getting emails any time that happens because we're code owners. Therefore I think the option (1) would work more quietly and create less email traffic. In this case, having labels, platformAutomerge and GHA workflow seems redundant to me.

I don't have a strong preference and I don't confidently know how things would work with PRs so I may be happy with the option (2) as well but in this case let's please set "automergeType": "pr" instead of "branch" to make the intent clear.

@@ -0,0 +1,17 @@
name: auto-merge
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be called auto-approve instead of auto-merge? (If you agree, auto-merge is mentioned in the file name and twice in the names in this file, excluding the label.)
It may also be helpful to add a comment saying that auto-approve in combination with Renovate settings will lead to automerging of the PR.

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.

2 participants