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

Enable renovatebot for this repo (and contrib) #8936

Closed
codeboten opened this issue Nov 16, 2023 · 8 comments · Fixed by #9009
Closed

Enable renovatebot for this repo (and contrib) #8936

codeboten opened this issue Nov 16, 2023 · 8 comments · Fixed by #9009

Comments

@codeboten
Copy link
Contributor

codeboten commented Nov 16, 2023

After spending more time testing out renovatebot for this repository, I think it's an overall improvement over what we currently have.

Dependabot workflow

  1. dependabot opens a new PR for every dependency for every module in the repo (this is super noisy)
  2. an approver/maintainer kicks off the "Automation - Dependabot PR" action
  3. that action gets a list of all dependabot PRs, runs update-dep and gotidy

This works in some cases, but often times, the PR produced runs into a variety of issues.

Issues w/ this workflow

  1. any of the dependency requires code changes causing a failure in the CI
  2. some deps are currently known to cause issues (exp update for example causes prometheus issues)
  3. dependabot runs into a limit of components it supports, which we run into in the contrib repo.
  4. it's annoying to have someone remember to trigger a CI manually, this could be automated

An alternative would be to use Renovatebot.

Renovatebot workflow

  1. A single PR per dependency or per monorepo that combines multiple deps is opened for all components.
  2. The workflow "Project: Tidy" is run as part of the CI for the PR above to run gotidy and pushes a change to the PR's branch

Issues w/ this workflow

Now ideally, this is where an approver could review the PR and approve it to get it merged. Unfortunately, it isn't quite so simple. Once the "Project: Tidy" has completed, the change does not trigger CI again. This is because the standard token (GITHUB_TOKEN) used to push the change does not have the ability to trigger github actions. This is a known limitation of the token. The solution is to obtain a token with write access to the repository. However, there isn't a way to limit write access of a token to only a branch.

A write token has access to various other things, including publishing release. This was specifically called out as a problem when I opened a community request to obtain such a token. So, there are a few options we have here and I would like other contributors' input on what steps we should follow here:

Options

  1. we could obtain a write token, but this require us to not publish any binary artifacts in this repository. Currently the only thing we publish is the builder, which I've proposed we should move to releases some time ago.
  2. we keep the current dependabot workflow
  3. we switch to renovatebot, and reviewers have to close/re-open (or we could do things like label/unlabel the PR) to trigger CI
@codeboten codeboten changed the title Enable renovatebot for this repo Enable renovatebot for this repo (and contrib) Nov 16, 2023
@TylerHelmuth
Copy link
Member

TylerHelmuth commented Nov 16, 2023

@codeboten would it be possible to have another workflow monitoring the renovate PR, see the tidy happen, and then do something to the PR to force the CI to rerun like add an extra label?

Thats all pretty convoluted, but would remove the need for a human to intervene.

@codeboten
Copy link
Contributor Author

codeboten commented Nov 16, 2023

@TylerHelmuth that might be possible, i tried using a follow up step in the action to remove a label, which triggered a new event but it didn't trigger CI. I'm open to other suggestions!

@TylerHelmuth
Copy link
Member

TylerHelmuth commented Nov 16, 2023

I think we'd have to add watches to all the workflows we'd want triggered by the label. Similar to the changelog workflow.

@evan-bradley
Copy link
Contributor

In my experience, most interactions with the GitHub API from a bot or action are explicitly filtered out from action triggers to prevent workflows from recursively triggering themselves. That said, maybe there's some way we can get clever. I think the changelog label works because it's a human adding/removing the label.

For option 1, I don't have any issue with moving ocb to be published from the releases repo. I think having all binaries available in a single spot makes it clearer where to get our built artifacts, so I'm good with restricting binaries to only the releases repo. Are we confident that we can adequately address all of the security concerns with the token's permissions?

@codeboten
Copy link
Contributor Author

Are we confident that we can adequately address all of the security concerns with the token's permissions?

@evan-bradley I believe the only requirement identified was to not publish any executable artifacts from the repository, and to have a workflow that checks this. I don't know if this would require us to also validate that no previously released binary has changed, or that we remove them altogether. As you can see though, the PAT with contents permissions is quite permissive: https://docs.github.com/en/rest/overview/permissions-required-for-fine-grained-personal-access-tokens?apiVersion=2022-11-28#repository-permissions-for-contents

That said, maybe there's some way we can get clever. I think the changelog label works because it's a human adding/removing the label.

I'm all ears if you have suggestions on how to get clever. You're right that the label works when someone interacts with them.

codeboten pushed a commit that referenced this issue Nov 28, 2023
Finally rid us of the hundreds of notifications.

Closes
#4885
Closes #8936

---------

Signed-off-by: Alex Boten <aboten@lightstep.com>
pantuza pushed a commit to pantuza/opentelemetry-collector that referenced this issue Dec 8, 2023
Finally rid us of the hundreds of notifications.

Closes
open-telemetry#4885
Closes open-telemetry#8936

---------

Signed-off-by: Alex Boten <aboten@lightstep.com>
@yurishkuro
Copy link
Member

@codeboten can I ask a few follow-ups?

  1. I seem to recall that dependabot allows adding a custom step, so that tidy could be run automatically in the same PR preparing phase. Does Renovate not support such option?
  2. Is it go mod tidy or something more custom? In Jaeger I never needed to run go mod tidy after dependabot.
  3. Did you investigate Forking Renovate app, which does not require write permissions to the repo?

@codeboten
Copy link
Contributor Author

  1. I seem to recall that dependabot allows adding a custom step, so that tidy could be run automatically in the same PR preparing phase. Does Renovate not support such option?

Renovate supports this as well, however we need to run make gotidy to run tidy in all the submodules. At the time of the transition to renovate, running a custom command like this would require a custom executor which would require the project hosting a runner somewhere. The same limitation existed for dependabot

  1. Is it go mod tidy or something more custom? In Jaeger I never needed to run go mod tidy after dependabot.

This is a good question. I can't remember the details of why make gotidy which runs mod tidy across all packages in the monorepo is needed when a package is upgraded. I think it has to do w/ the dependencies on packages within the repo. You can see an example of a failure https://github.com/open-telemetry/opentelemetry-collector/actions/runs/9464393703/job/26071646839 if you have time to investigate it. We'd love to not have to run custom commands in the workflows.

  1. Did you investigate Forking Renovate app, which does not require write permissions to the repo?

Yes. The original prototyping of using renovate was done w/ forking renovate in hope that it would allow us to run commands needed on the fork. I seem to recall there were permission issues with forking renovate not being able to update PRs, but I can't remember or find the reasoning for the switch documented.

@yurishkuro
Copy link
Member

Thanks for background, @codeboten. I enabled Renovate in Jaeger yesterday, but then switched to Forking flavor, because the permissions on the main one are insane. I know Forking is not able to auto-merge, but we were not planning using that anyway, and everything else seems to work fine: it updates its own PRs and a "Dashboard" issue, and we don't have a need for tidy step. Also Forking flavor manages its own branches, the main flavor was leaving them around (apparently the insane perms it gets are still not enough to delete branches, according to docs I saw somewhere).

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 a pull request may close this issue.

4 participants