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

Switch to Renovate from Dependabot for remaining dependencies #9459

Merged
merged 15 commits into from
Aug 7, 2024

Conversation

timja
Copy link
Member

@timja timja commented Jul 13, 2024

See jenkins-infra/helpdesk#4173

We were already using renovate for JavaScript dependencies, this switches us to using it for the remaining ones.
Primary motivator is buggy handling of SNAPSHOTS in dependabot.

Testing done

Tested on my org fork (my personal fork was getting auth failures for some reason):
https://github.com/timja-org/jenkins/pulls?q=sort%3Aupdated-desc+is%3Apr+is%3Aopen

image

I downgraded remoting on my fork and verified it got upgraded correctly: timja-org#6

Full result of detected packages / package managers:
https://developer.mend.io/github/timja-org/jenkins

Proposed changelog entries

N/A

Proposed upgrade guidelines

N/A

Submitter checklist

Desired reviewers

N/A

Before the changes are marked as ready-for-merge:

Maintainer checklist

@timja timja requested review from basil and NotMyFault July 13, 2024 12:25
@timja timja added the skip-changelog Should not be shown in the changelog label Jul 13, 2024
schedule:
interval: "daily"
# Include only security updates and exclude version updates.
open-pull-requests-limit: 0
Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Member

@basil basil Aug 2, 2024

Choose a reason for hiding this comment

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

I don't think this ever worked

Sad, given that it cost me so much to get that change accepted. I still think this is something that we want to automate, and we essentially do it manually for every LTS release, including recent ones.

Copy link
Member Author

Choose a reason for hiding this comment

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

Restored in 74eee38

had to debug a fair but and used:

RENOVATE_CONFIG_FILE=.github/renovate.json renovate --token=$GITHUB_TOKEN --schedule=""  --require-config=ignored --dry-run=full timja-org/jenkins

Copy link
Member Author

Choose a reason for hiding this comment

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

Here are the LTS PRs that would be created: https://github.com/timja-org/jenkins/pulls?q=sort%3Aupdated-desc+is%3Apr+is%3Aopen+label%3Ainto-lts

(some remoting ones would disappear by backporting this PR to LTS)

Copy link
Contributor

@MarkEWaite MarkEWaite left a comment

Choose a reason for hiding this comment

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

The proposed list of updates seems very reasonable to me.

The args4j 2.37 release that is included in the list of proposed upgrades was released in March 2024. Good that renovate detected it. I've not done any investigation to see if the upgrade is safe, since it will need evaluation in remoting as well as Jenkins core.

@basil
Copy link
Member

basil commented Jul 14, 2024

Is this able to upgrade detached plugins like Dependabot?

@timja
Copy link
Member Author

timja commented Jul 14, 2024

Is this able to upgrade detached plugins like Dependabot?

It's upgrading war/pom.xml but not test/pom.xml for some reason:

timja-org#8

https://github.com/timja-org/jenkins/blob/b673f0d8108d2d03b5bb34d536060dc6741b107b/test/pom.xml#L353

Looking in to it


Pom files found:

DEBUG: Matched 9 file(s) for manager maven: .mvn/extensions.xml, bom/pom.xml, cli/pom.xml, core/pom.xml, coverage/pom.xml, pom.xml, war/pom.xml, websocket/jetty10/pom.xml, websocket/spi/pom.xml

I checked all poms and only test/pom.xml is missing


I see this setting in logs:

DEBUG: Found repo ignorePaths
{
  "ignorePaths": [
    "**/node_modules/**",
    "**/bower_components/**",
    "**/vendor/**",
    "**/examples/**",
    "**/__tests__/**",
    "**/test/**",
    "**/tests/**",
    "**/__fixtures__/**"
  ]
}

@timja
Copy link
Member Author

timja commented Jul 14, 2024

Is this able to upgrade detached plugins like Dependabot?

Yes, all working now that test isn't ignored: timja-org#8

@timja
Copy link
Member Author

timja commented Jul 14, 2024

It is now trying to upgrade old-remoting-for-test and remoting.minimum.supported.version I'll see if I can stop that

@timja
Copy link
Member Author

timja commented Jul 14, 2024

It is now trying to upgrade old-remoting-for-test and remoting.minimum.supported.version I'll see if I can stop that

@viceice is it possible to stop it updating those two intentionally old deps?

timja-org#10
timja-org#9

@viceice
Copy link
Member

viceice commented Jul 14, 2024

yes, with a package rule and an allowed version filter. you can also disable that package or require dashboard approval.

@timja
Copy link
Member Author

timja commented Jul 14, 2024

yes, with a package rule and an allowed version filter. you can also disable that package or require dashboard approval.

Hmm we still want the regular dependency one to work, timja-org#6
Just not the 'minimum version' or the 'old version to test'.

Would that still work?

I couldn't see a way to filter out a dependency from certain paths

Dashboard approval could maybe work

@viceice
Copy link
Member

viceice commented Jul 14, 2024

yes, with a package rule and an allowed version filter. you can also disable that package or require dashboard approval.

Hmm we still want the regular dependency one to work, timja-org#6
Just not the 'minimum version' or the 'old version to test'.

Would that still work?

I couldn't see a way to filter out a dependency from certain paths

Dashboard approval could maybe work

this can do it:
https://docs.renovatebot.com/configuration-options/#matchfilenames

@timja
Copy link
Member Author

timja commented Jul 14, 2024

@viceice
Copy link
Member

viceice commented Jul 15, 2024

This not right? https://github.com/timja-org/jenkins/blob/37f95213954136508d215a4972d701bae1689a8b/.github/renovate.json#L71

no. you're missing an action. that rule matches the file and excludes that dep. but you don't say what renovate should do. so add enabled:false to disable or allowed version to restrict only by version.

@timja
Copy link
Member Author

timja commented Jul 15, 2024

Thanks that fixed one of them.

Surprisingly it didn't close this one though: timja-org#9

(It wasn't picked up when the entire file was ignored before adjusting ignore patterns)

Any idea?

@viceice
Copy link
Member

viceice commented Jul 15, 2024

I'll try to have a look again tomorrow, currently on holiday 🙃

@timja
Copy link
Member Author

timja commented Jul 18, 2024

@viceice you have a chance to check it?

@viceice
Copy link
Member

viceice commented Jul 18, 2024

@viceice you have a chance to check it?

not yet, sorry. feel free to open a support discussion on the renovate repo. maybe one of my co-maintainer will have more time to look at it. I'm back on track on Monday.

@timja
Copy link
Member Author

timja commented Jul 31, 2024

Hey @viceice any thoughts?

@timja
Copy link
Member Author

timja commented Aug 2, 2024

raised config help at renovatebot/renovate#30555

@timja
Copy link
Member Author

timja commented Aug 2, 2024

All working now, @basil / others any concerns?

Comment on lines 70 to 72
"matchFileNames": ["test/pom.xml", ".mvn/maven.config"],
"matchPackageNames": ["org.jenkins-ci.main:remoting"],
"enabled": false
Copy link
Member

Choose a reason for hiding this comment

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

this rule doesn't match pom.xml, so renovate still suggests an update for org.jenkins-ci.main:remoting inside the root pom.xml

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah there's one org.jenkins-ci.main:remoting that we want updated, the remoting.version one which is the latest version.

We don't want the 'old version for testing', and the 'minimum supported version' updated. Which is all working with this setup now by moving it to .mvn/maven.config

@viceice
Copy link
Member

viceice commented Aug 2, 2024

Thanks that fixed one of them.

Surprisingly it didn't close this one though: timja-org#9

(It wasn't picked up when the entire file was ignored before adjusting ignore patterns)

Any idea?

See my code comment for the solution

https://github.com/jenkinsci/jenkins/pull/9459/files/5895e0ebe23dea21644e42d3a7c32e4f63295615#diff-f82f7d36a61d22f640c25bdb8b0435bf9cb38679d99d5f460753fa668ee7cdb3R72

.github/renovate.json Outdated Show resolved Hide resolved
@timja
Copy link
Member Author

timja commented Aug 5, 2024

I hit a bunch of weird behaviour when I added baseBranches in, matchPackageNames wildcarding and regex didn't seem to work.

I've managed to fix almost everything although this rule isn't working right now:

    {
      "matchPackageNames": ["net.jcip:jcip-annotations"],
      "matchDatasources": ["maven"],
      "enabled": false,
      "description": "maven-metadata.xml is missing for this really old package which is required by renovate"
    }

its still complaining about not being able to resolve it and if I just drop it to master or stable branch only then its fine with that rule (not a blocker anyway it just fixes a warning).

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.

looks good

Copy link
Member

@basil basil left a comment

Choose a reason for hiding this comment

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

Thanks for the PR!

@timja
Copy link
Member Author

timja commented Aug 7, 2024

/label ready-for-merge


This PR is now ready for merge, after ~24 hours, we will merge it if there's no negative feedback.

Thanks!

@comment-ops-bot comment-ops-bot bot added the ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback label Aug 7, 2024
@github-actions github-actions bot added the unresolved-merge-conflict There is a merge conflict with the target branch. label Aug 7, 2024
Copy link
Contributor

github-actions bot commented Aug 7, 2024

Please take a moment and address the merge conflicts of your pull request. Thanks!

@github-actions github-actions bot removed the unresolved-merge-conflict There is a merge conflict with the target branch. label Aug 7, 2024
@basil basil changed the title Switch to renovate from dependabot for remaining dependencies Switch to Renovate from Dependabot for remaining dependencies Aug 7, 2024
@basil basil merged commit 9b1b1c6 into jenkinsci:master Aug 7, 2024
16 checks passed
@basil
Copy link
Member

basil commented Aug 7, 2024

There is a problem visible in #9529: Renovate is trying to update the GitHub Action to the latest commit, not the latest tagged commit. We want the same behavior as Dependabot, where updates are only offered once a commit is tagged.

@basil
Copy link
Member

basil commented Aug 7, 2024

There is a problem visible in https://github.com/jenkinsci/jenkins/pulls: Renovate is trying to backport all Java library updates, not just security updates. We want the same behavior we were trying to enable in Dependabot, where only security updates are backported.

@viceice
Copy link
Member

viceice commented Aug 7, 2024

add a version tag comment, that is understood by dependabot and renovate. then they follow and update that tag.

eg uses: org/repo@<sha> # v1.2.3

https://docs.renovatebot.com/modules/manager/github-actions/

@timja timja deleted the switch-to-renovate branch August 7, 2024 20:34
@basil
Copy link
Member

basil commented Aug 7, 2024

There is a problem visible in https://github.com/jenkinsci/jenkins/pulls: Renovate is trying to backport all Java library updates, not just security updates. We want the same behavior we were trying to enable in Dependabot, where only security updates are backported.

@timja Can you try re-adding the security updates with osvVulnerabilityAlerts using matchBaseBranches inside packageRules (per renovatebot/renovate#20542 (reply in thread))? vulnerabilityAlerts seems to be a non-starter, as it uses the GitHub Dependabot alerts (which only work on the default branch).

@basil
Copy link
Member

basil commented Aug 14, 2024

@timja https://github.com/jenkinsci/jenkins-test-harness/releases/tag/2254.vcff7a_d4969e5 was released 7 hours ago, but Renovate does not seem to be offering it to Jenkins core, even after manually triggering Renovate. Dependabot is offering the update in other repositories, such as jenkinsci/plugin-pom#984.

@timja
Copy link
Member Author

timja commented Aug 14, 2024

@timja jenkinsci/jenkins-test-harness@2254.vcff7a_d4969e5 (release) was released 7 hours ago, but Renovate does not seem to be offering it to Jenkins core, even after manually triggering Renovate. Dependabot is offering the update in other repositories, such as jenkinsci/plugin-pom#984.

resolved

@timja Can you try re-adding the security updates with osvVulnerabilityAlerts using matchBaseBranches inside packageRules (per renovatebot/renovate#20542 (reply in thread))? vulnerabilityAlerts seems to be a non-starter, as it uses the GitHub Dependabot alerts (which only work on the default branch).

I'm trying to get it working, haven't managed yet though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback skip-changelog Should not be shown in the changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants