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

Split and fix renovate config #767

Merged
merged 3 commits into from
Jul 4, 2024
Merged

Split and fix renovate config #767

merged 3 commits into from
Jul 4, 2024

Conversation

nadiamoe
Copy link
Member

@nadiamoe nadiamoe commented Jul 4, 2024

When selfhosted, renovate requires a separate config file (ref). This PR splits out self-hosted options to a different file, and points renovate itself to it. Additionally:

@nadiamoe nadiamoe requested a review from a team as a code owner July 4, 2024 10:12
@nadiamoe nadiamoe changed the title Split renovate config Split and fix renovate config Jul 4, 2024
@mem
Copy link
Contributor

mem commented Jul 4, 2024

When selfhosted, renovate requires a separate config file (ref). This PR splits out self-hosted options to a different file, and points renovate itself to it. Additionally:

  • Migrates repo config to json5 (as it allows comments)

I avoided using json5 because pretty much nothing supports it. There are good reasons for that. If you want comments in json, can I suggest that you use jsonnet? Every json document is a valid jsonnet input.

And it was disappointing to find that renovate explicitly declined supporting YAML as the configuration format. Not that I like YAML, but I admit that it's better than JSON at this kind of thing.

I copied the configuration from the docs, thinking that was going to provide a simpler path forward. I was so mistaken...

I noticed in the renovate docs that there's some kind of staging / testing functionality. Do you have any experience with that? I would love to avoid the PR / merge / diagnose / PR / ... cycle if possible.

Copy link
Contributor

@mem mem left a comment

Choose a reason for hiding this comment

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

It's OK, I guess.

I would like to understand why for the changes. I can figure out the what myself just by reading thru the changes.

@@ -48,7 +50,7 @@
".*\\.mk$"
],
"matchStrings": [
"\\bghcr\\.io/(?<depName>grafana/grafana-build-tools):(?<currentValue>\\S+)\\b"
"ghcr.io/grafana/grafana-build-tools:(?<currentValue>\\S+)"
Copy link
Contributor

Choose a reason for hiding this comment

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

I do not like to play loose with regular expressions.

What's the problem with correctly anchoring the regular expression? And what's the problem with correctly escaping things that need escaping?

I almost threw up when I realized that renovate does global matches, because that does make writing regular expressions much more complicated, e.g. you cannot reasonably anchor at the beginning or end of a line.

Copy link
Contributor

Choose a reason for hiding this comment

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

Incidentally, the reason why they do global matches is because they encourage you do to what you are removing here: use named captures to provide configuration values, so I don't quite understand why you are removing that instead of just changing the named capture so that it starts before ghcr.

Copy link
Member Author

Choose a reason for hiding this comment

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

What's the problem with correctly anchoring the regular expression?

This is not a strong opinion.

I've been using renovate and its terrible (and yet awesome) regexes for several years, and I've gone through efforts of making regexes very specific and robust so they don't catch things they shouldn't by mistake. Then I stopped doing it. Since I do the simplest, dumbest, possible regex I've noticed two things:

  • Dumb regexes have never caught up things they shouldn't. Not a single time.
  • I can actually read the dumb regex three days after I write it.

For those two reasons I keep the shortest, simplest possible one that I cannot anticipate breaking.

use named captures to provide configuration values

Also not a strong opinion. I've found prematurely generalizing managers also obscures the reasoning for the rule being added, and also adds cognitive complexity (just like the regex).

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you point me to the documentation that explains what's going on here? I can see that you are pointing the action at this file. How is the other one used then?

My guess: you are saying that this file is the configuration file for the GitHub App, and the other file is the configuration file for renovate (i.e. renovate doesn't care about this one). Is that it?

Copy link
Member Author

Choose a reason for hiding this comment

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

I added a link in the PR, but probably not in the most readable way. Sorry about that.

Yep, this is the "self hosted configuration", as renovate refers to it. It is documented here: https://docs.renovatebot.com/self-hosted-configuration/

Note the following paragraph:

Do not put the self-hosted config options listed on this page in your "repository config" file (renovate.json for example), because Renovate will ignore those config options, and may also create a config error issue.

The repository configuration is described, in turn, here: https://docs.renovatebot.com/configuration-options/. That one is not referenced anywhere because it is one of the (many, arguably too many) default paths renovate looks for after cloning the repo.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is a bit more obvious when renovate does not live in the same repo it manages (e.g. when you run the bot itself on k8s), as the self-hosted configuration lives next to the bot, and not in the repository.

Copy link
Contributor

Choose a reason for hiding this comment

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

Argh...

Thanks for the explanation.

The other day when I was working on this I started to hate renovate because the documentation is really bad (having a lot of documentation is not the same thing as having good documentation). Now I hate it even more. I did see that line you refer to, I did not (obviously) pick up on the subtlety it's trying to express.

I guess I have to write a PR for the repo where I got the example from...

"customType": "regex",
"datasourceTemplate": "github-tags",
"depNameTemplate": "ghcr.io/grafana/grafana-build-tools",
"datasourceTemplate": "docker",
Copy link
Contributor

Choose a reason for hiding this comment

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

It's OK for me either way, but why is this better?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think it is better, it should work the same. It is perhaps a bit more obvious. When I first saw this I wondered: Is there a reason to not do the obvious thing and look at the OCI registry for OCI image tags?

Copy link
Contributor

Choose a reason for hiding this comment

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

The reason is that I was trying to test the configuration before actually committing anything to the repo, and with docker it was failing in even less useful ways than with github-tags.

I do think docker is the right thing to do here, because in this case the tag might be published a long time before the docker image is available.

Copy link
Member Author

Choose a reason for hiding this comment

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

That's interesting, I'll follow up on that to see why that might happen.

@nadiamoe
Copy link
Member Author

nadiamoe commented Jul 4, 2024

I wish I had. My experience is usually iterating force-pushes to main, as sad as that is. The fact that we're trying to troubleshoot renovate running as an app is pretty unconvenient, because we cannot simply run renovate locally (which is my plan B when forcepushes are not acceptable).

I tried doing that. I failed miserably. I still don't understand why. I would have been happy with a "--dry-run" or "--dont-create-commits" option.

Thanks!

EDIT: Sorry, I clicked the wrong button. I wanted to quote-reply and I clicked "edit". I have no idea how to revert that. 🤷🏽

@mem mem merged commit aaa8c72 into main Jul 4, 2024
2 checks passed
@mem mem deleted the split-renovate branch July 4, 2024 20:20
@mem mem mentioned this pull request Jul 4, 2024
nadiamoe added a commit that referenced this pull request Jul 15, 2024
* Chore(deps): Bump google.golang.org/grpc from 1.63.2 to 1.64.0
* grpc: nolint deprecated grpc options
* Build(deps): Bump the prometheus-go group across 1 directory with 2 updates
* http: rename `promconfig.Header` to `promconfig.ProxyHeader`
* Add the basic stuff to support a browser check type (#737)
* k6runner/test: ensure logs are sent to loki when runner reports user errors
* k6runner: send logs even if metrics are malformed
* Build(deps): Bump github.com/prometheus/common
* Bump k6 version using renovate (#745)
* Dispatch renovate workflow manually (#746)
* Update renovate (#748)
* Update dependency grafana/k6 to v0.52.0 (#749)
* Fix renovate configuration (#751)
* Update github.com/grafana/loki/pkg/push digest to 04bc3a4 (#752)
* Update module github.com/dmarkham/enumer to v1.5.10 (#754)
* Update module github.com/sirupsen/logrus to v1.9.3 (#755)
* Update module github.com/stretchr/testify to v1.9.0 (#759)
* Update dependency grafana/grafana-build-tools to v0.15.0 (#756)
* Correct the listen address to scrape metrics to port 4050. (#743)
* Build(deps): Bump google.golang.org/grpc from 1.64.0 to 1.65.0
* Update module github.com/securego/gosec to v2
* Split and fix renovate config (#767)
* Update ghcr.io/grafana/grafana-build-tools Docker tag to v0.15.1 (#770)
* add validation case for browser (#773)
* Update module gotest.tools/gotestsum to v1.12.0
* Update module go.k6.io/k6 to v0.52.0
* Update golang.org/x/exp digest to 7f521ea
* Update module golang.org/x/net to v0.27.0 (#775)
* Update module github.com/securego/gosec to v2
* Update module gotest.tools/gotestsum to v1.12.0
* Update module github.com/spf13/afero to v1.11.0 (#758)
* Update module github.com/golangci/golangci-lint to v1.59.1
* Update github.com/grafana/loki/pkg/push digest to 6da364e
* Update module github.com/securego/gosec to v2
* Route browser checks to the browser prober (#780)
* Update module google.golang.org/grpc to v1.65.0 (#761)
* ci/renovate: limit gomod digest updates to once every two weeks (#785)
* Update golang.org/x/exp digest to 46b0784 (#781)
* cmd: default to sm-k6 binary
* Dockerfile: copy sm-specific k6 as sm-k6 instead of just k6
* k6runner/test: pass address of expectErrorAs to errors.As
* k6runner: log errors encountered by logfmt parser
* Update github.com/grafana/loki/pkg/push digest to 7c86e65
* Add browser limits and class (#787)
* Update module github.com/prometheus/prometheus to v0.53.1 (#790)
* Report browser check's class as browser (#788)
* Update github.com/grafana/loki/pkg/push digest to 527510d (#793)
* Update github.com/securego/gosec/v2 digest to 4487a0c

Signed-off-by: Ro Santalla <[email protected]>
@nadiamoe nadiamoe mentioned this pull request Jul 15, 2024
nadiamoe added a commit that referenced this pull request Jul 15, 2024
* Chore(deps): Bump google.golang.org/grpc from 1.63.2 to 1.64.0
* grpc: nolint deprecated grpc options
* Build(deps): Bump the prometheus-go group across 1 directory with 2 updates
* http: rename `promconfig.Header` to `promconfig.ProxyHeader`
* Add the basic stuff to support a browser check type (#737)
* k6runner/test: ensure logs are sent to loki when runner reports user errors
* k6runner: send logs even if metrics are malformed
* Build(deps): Bump github.com/prometheus/common
* Bump k6 version using renovate (#745)
* Dispatch renovate workflow manually (#746)
* Update renovate (#748)
* Update dependency grafana/k6 to v0.52.0 (#749)
* Fix renovate configuration (#751)
* Update github.com/grafana/loki/pkg/push digest to 04bc3a4 (#752)
* Update module github.com/dmarkham/enumer to v1.5.10 (#754)
* Update module github.com/sirupsen/logrus to v1.9.3 (#755)
* Update module github.com/stretchr/testify to v1.9.0 (#759)
* Update dependency grafana/grafana-build-tools to v0.15.0 (#756)
* Correct the listen address to scrape metrics to port 4050. (#743)
* Build(deps): Bump google.golang.org/grpc from 1.64.0 to 1.65.0
* Update module github.com/securego/gosec to v2
* Split and fix renovate config (#767)
* Update ghcr.io/grafana/grafana-build-tools Docker tag to v0.15.1 (#770)
* add validation case for browser (#773)
* Update module gotest.tools/gotestsum to v1.12.0
* Update module go.k6.io/k6 to v0.52.0
* Update golang.org/x/exp digest to 7f521ea
* Update module golang.org/x/net to v0.27.0 (#775)
* Update module github.com/securego/gosec to v2
* Update module gotest.tools/gotestsum to v1.12.0
* Update module github.com/spf13/afero to v1.11.0 (#758)
* Update module github.com/golangci/golangci-lint to v1.59.1
* Update github.com/grafana/loki/pkg/push digest to 6da364e
* Update module github.com/securego/gosec to v2
* Route browser checks to the browser prober (#780)
* Update module google.golang.org/grpc to v1.65.0 (#761)
* ci/renovate: limit gomod digest updates to once every two weeks (#785)
* Update golang.org/x/exp digest to 46b0784 (#781)
* cmd: default to sm-k6 binary
* Dockerfile: copy sm-specific k6 as sm-k6 instead of just k6
* k6runner/test: pass address of expectErrorAs to errors.As
* k6runner: log errors encountered by logfmt parser
* Update github.com/grafana/loki/pkg/push digest to 7c86e65
* Add browser limits and class (#787)
* Update module github.com/prometheus/prometheus to v0.53.1 (#790)
* Report browser check's class as browser (#788)
* Update github.com/grafana/loki/pkg/push digest to 527510d (#793)
* Update github.com/securego/gosec/v2 digest to 4487a0c

Signed-off-by: Ro Santalla <[email protected]>
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