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

SOLR-17571: Introduce dependabot #2880

Draft
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

malliaridis
Copy link
Contributor

@malliaridis malliaridis commented Nov 20, 2024

https://issues.apache.org/jira/browse/SOLR-17571

Description

With the introduction of Version catalogs we can make use of dependabot and replace our current self-hosted bot. This allows us to run regular dependency and security updates directly on the project without the need of maintaining an external bot.

Solution

The solution introduces multiple changes in workflows and enables GitHub features that are used by dependabot.

Dependabot does not support execution of gradle tasks, so it has to be realized with GitHub workflows. This means, that a new job is introduced that executes the writeLocks and updateLicenses gradle tasks, and commits the changes. This job is the lock-and-verify job. The lock-and-verify job is only executed for dependabot PRs and skipped for all other PRs.

All current workflows that run tests with the PR require the updated locks and checksum from the new commit in order to succeed. So they depend on the lock-and-verify job. One option to realize this dependency would be to use the workflow_run event, but this would require us to remove the on.pull_request event in order for the jobs to be executed after the commit is made. This however would not execute the workflows for any user-created PR anymore, as the lock-and-verify job is not executed there to trigger them, nor does it execute on every commit.

Therefore, all jobs that run tests on the PR are merged into a single workflow called "Pull Request Checks" (pull-request-checks.yml). These jobs are then organized in sequence and in parallel and selectively executed depending on the labels set by our labeler, to achieve the same selective execution as with the workflows.

Besides that, dependabot security update PRs require the feature "Dependabot security updates" from GitHub to work, which should be explicitly enabled in the Code Security settings of the project.

The solution introduces a configuration for dependabot that checks for security updates daily and creates PRs with dependency updates if they are security-related (unlimited).

Additionally, it creates regular dependency updates bi-weekly and up to 10 PRs (for the beginning), grouping dependencies together based on our version catalog and custom dependency groups that are created at the dependabot configuration for related dependencies (like Apache Calcite dependencies).

The inline comments contain more details about the job execution order. The current implementation runs or skips the write-and-verify job, then the pre-commit job if write-and-verify succeeds or is skipped, and then all other jobs in parallel if pre-commit succeeds. Note that pre-commit may also run in parallel with all other jobs to have the same execution order like before.

Testing

These changes are tested in a separate temporary project that can be found here.

What needs to work:

  • On a dependabot PR the versions.lock and checksums are updated together with the dependency version
  • On a dependabot PR if a user commits changes on top of existing changes, it does not retrigger the lock-and-verify job and fail because of that
  • On a dependabot PR if a user commits changes on top of existing changes, it still executes the existing jobs for all the tests, based on the files changed (e.g. script or solrj tests)
  • On a dependabot PR the existing workflows / jobs do not fail due to pending updates in lock file or checksums
  • On a user PR the dependabot jobs are not triggered
  • On a user PR the workflows / jobs are executed as before
  • Workflow execution times are not drastically impacted (needs comparison)
  • Dependabot rebase or recreate commands work as expected on dependabot PRs (at least one needs to work). That is, the final state is including all necessary update changes
  • PRs from forks still work the same way (needs testing)
  • Security updates can be distinguished from regular updates
  • Security updates are not limited
  • Regular dependency updates are limited (e.g. max 10 PRs)
  • Security updates are checked regularly (e.g. daily)
  • Regular updates are checked regularly (e.g. bi-weekly)

Checklist

Please review the following and check all that apply:

  • I have reviewed the guidelines for How to Contribute and my code conforms to the standards described there to the best of my ability.
  • I have created a Jira issue and added the issue ID to my pull request title.
  • I have given Solr maintainers access to contribute to my PR branch. (optional but recommended, not available for branches on forks living under an organisation)
  • I have developed this patch against the main branch.
  • I have run ./gradlew check.
  • I have added tests for my changes.
  • I have added documentation for the Reference Guide

Copy link
Contributor

@janhoy janhoy left a comment

Choose a reason for hiding this comment

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

Looks promising. Have a few comments

.github/workflows/gradle-precommit.yml Outdated Show resolved Hide resolved
.github/workflows/gradle-precommit.yml Outdated Show resolved Hide resolved
@malliaridis
Copy link
Contributor Author

The current configuration has multiple issues that I will fix in the upcoming changes. Are there any specific needs for how to treat security updates (dependencies with vulnerabilities)? Or should they simply be treated like any other dependency? This would simplify the dependabot config a bit if we want to treat them the same way.

Additionally, running dependabot with a working configuration and no PR limit creates over 70 PRs (gradle plugin updates included). Should we limit it for the introduction to avoid spam?

@janhoy
Copy link
Contributor

janhoy commented Nov 21, 2024

Are there any specific needs for how to treat security updates (dependencies with vulnerabilities)?

We have not distinguished these before, and such CVEs are already public anyway. Curious what the commit message will look like, i.e. will it highlight that it is a security upgrade?

Sometimes a vulnerable dependency makes Solr vulnerable, and sometimes that warrants a solr bugfix release. Committers should probably monitor dependabot security PRs and make such considerations on a day to day basis. Not sure if we need any extra processes for it. A bonus that we get a "security" label on those PRs!

@janhoy
Copy link
Contributor

janhoy commented Nov 21, 2024

no PR limit creates over 70 PRs

If possible, I'd rate-limit this on the first few days so we have a chance to fine-tune the bot.

.github/dependabot.yml Outdated Show resolved Hide resolved
Since dependabot is creating a commit, it is necessary to wait and retrieve the latest changes before running any other workflow.
@malliaridis
Copy link
Contributor Author

Sorry for the delayed update on this PR. I was testing on a separate project the configurations, and had to make further changes to make security updates actually work correctly.

The main issue we have with dependabot is that it commits the lock and checksum changes in a separate commit. This requires all the other workflows to wait in case there is a dependabot action pending, so that they can fetch the latest state.

I have also reduced the PR limit to 10 as requested, in the same commit.

@malliaridis
Copy link
Contributor Author

Current changes do not work for PRs outside the repository, so more work needs to be done on that "checkout latest changes" configuration.

@malliaridis
Copy link
Contributor Author

A bonus that we get a "security" label on those PRs!

I have also noticed that our labeler overrides the labels dependabot is setting. I couldn't find a way to configure the labeler so that it keeps the existing labels set by dependabot, or to disable it for dependabot PRs. It is also not possible from what I saw to use the PR title prefix or something similar to set the security label. Dependabot can also not be configured to use different branch names for security updates, otherwise that could be used instead.

@malliaridis malliaridis force-pushed the SOLR-17571/introduce-dependabot branch from d22993d to 5d193a0 Compare November 29, 2024 00:03
@janhoy
Copy link
Contributor

janhoy commented Nov 30, 2024

I couldn't find a way to configure the labeler so that it keeps the existing labels set by dependabot

I did a search and I believe you can make the entire labeler workflow conditional on labels through a syntax like

if: !contains(github.event.issue.labels.*.name, 'dependencies')

@malliaridis
Copy link
Contributor Author

@janhoy I was thinking of this solution, but disabling the labeler job would prevent it from updating the labels if specific files would be changed by a user in a dependabot PR (for fixing any breaking changes). The current solution I found is a bit more complex, so I'll explain it in the PR description. After what is necessary, I would also consider updating renovatebot if the updates are not acceptable.

@malliaridis
Copy link
Contributor Author

malliaridis commented Dec 1, 2024

One of the most significant changes in the latest updates is the workflow and job execution order. In the temporary project I have set up, the workflow execution https://github.com/malliaridis/solr-temp/actions/runs/12091788281 shows which jobs are executed in what order for a dependency update that does not alter any files. The initial PR is malliaridis/solr-temp#25.

The workflow execution https://github.com/malliaridis/solr-temp/actions/runs/12090400340 shows the job execution order for a user PR that alters solrj related files. The according PR is malliaridis/solr-temp#23.

Note that any failed crave jobs are related to the missing configuration for the crave-job in the self-hosted runner. Additionally, other labels were used for testing purposes. These were merged in this PR with existing labels. The added file patterns come from the previous workflows that were removed during the merge.

- name: Crave clone sources
run: crave clone create --projectID 39 /crave-devspaces/pipeline/runs/${GITHUB_RUN_ID}_${GITHUB_RUN_NUMBER}
- name: Checkout the correct branch
# TODO See if below run checks out latest commit
Copy link
Contributor Author

@malliaridis malliaridis Dec 1, 2024

Choose a reason for hiding this comment

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

This is a TODO that I have not been able to resolve yet due to the lack of information.

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