-
Notifications
You must be signed in to change notification settings - Fork 2.9k
feat: add support for go:generate directives in gomod #39163
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
feat: add support for go:generate directives in gomod #39163
Conversation
This change adds a `goGenerate` postUpdateOption which enables running `go generate ./...` after a dependency update. Frequently when dependencies are brought in via tools or using vendored code, and those dependencies are used for go:generate instructions, we want to update the generated code after the dependency has been updated. Adding this as a postUpdateCommand wasn't really satisfactory since that necessarily requires making a change in two spots (one in the allowed commands, one in the postUpdateCommand). The other reason this was undesirable was it would cause generate to run after every dependency update, regardless of language.
0348585 to
0685872
Compare
viceice
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM besides some security concerns 🤔
|
Yeah this does technically have some very valid risks, because we are now executing arbitrary commands Note that this does have similar risks to https://docs.renovatebot.com/modules/manager/gradle/#dependency-verification where running The code being executed (to generate code) could be maliciously attacked, similar to what we see with |
|
@jamietanna : What is the generally accepted practice for dealing with these types of issues in renovate dependency bumps? Possible options I can come up with:
If you think 1 is viable, I suspect the right spot to insert it would be similar to where goGetDirs is currently configured: https://docs.renovatebot.com/configuration-options/#gogetdirs and add an option such as: Where the valid config would be: if empty: run go generate after updating any dependency Thoughts? |
|
(Haven't forgotten about this - will get back to it this week - need to have a think) |
|
@jamietanna wanted to see if you had a chance to think through this? I'm happy to implement any solution you deem appropriate. |
|
Apologies, will try to take a look today 🙇 Out of interest, would you be hoping to run this on the Mend hosted Renovate app? Or are you self-hosting? (We'll probably not allow this by default on the Mend hosted Renovate app due to security risks) |
|
Given I'm wondering if we should instead "gate this" behind an opt-in flag. This would then - in the next breaking change - include all unsafe executions, and require self-hosting administrators to enable them after they've considered the implications. (I'm further documenting some of these risks in #39456) What that means for this PR is that I'd request that we create a PR before this goes in to create this global self-hosting configuration option to allowlist these unsafe options (which can start empty). Then, we'd tweak this PR to only execute this if it's in the allowlist. @viceice do you agree with this suggested path forwards? As mentioned, it's likely that we won't be enabling this by default with the Mend Developer Platform, for this reason. (But as noted in https://docs.renovatebot.com/configuration-options/#postupgradetasks, we'll allow trusted OSS repos + paying customers to enable this) But for folks self-hosting, it's completely fine for them to enable this. |
|
sounds good |
|
Thanks for the very detailed look into this. Just to answer some of the questions:
|
Awesome!
The use of By default, that list is empty, so no commands are usable. However, Renovate has a number of cases where Renovate will invoke commands that could contain unsafe code (as noted in #39456). These are controlled by the Renovate CLI maintainers, who will add commands (such as in this PR) and add them into i.e. These commands aren't allowlisted by an administrator, and by upgrading the Renovate CLI version, you're potentially including new commands that could be executed, and introduce risks of supply chain attacks However, the intent is that So to rephrase:
|
jamietanna
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for #39573 - mind integrating this PR in with it, so this is now an option (but not allowed by default)?
|
Thanks @jamietanna . I'm having trouble figuring out how to make the code coverage...higher. I think I have coverage for everything that I added. |
|
It also seems I'm missing something else, since this isn't working locally with these changes. Even when logging my config: I'm seeing: So there must be something else that I'm missing. |
|
Found the issue, it was the fact that the stage was set to |
|
@jamietanna : wondering if there's anything else that can help push this through? I'm having trouble figuring out how to get the test coverage higher given I believe my tests are covering the code paths. |
|
Update the branch once, and see if it helps |
|
Thanks @RahulGautamSingh that wasn't quite it. I had to update a test to catch one code path that wasn't being tested (unfortunately, and I commented about this above, it required modifying some possibly questionable code logic). |
|
OK, I've updated this PR, and given it a go against JamieTanna-Mend-testing/go-generate#3 - but it doesn't seem to have any changes committed for my generated changes - any idea? Logs: |
|
Ignore ☝️ |
In a future change, `GlobalConfig.get` will correctly pull in the defaults.
|
🎉 This PR is included in version 42.50.0 🎉 The release is available on:
Your semantic-release bot 📦🚀 |
|
@jamietanna thank you so much for your help and support getting this change through. Really appreciate the help. |
|
Thanks for the great work on this (and relevant prefactor changes) to get it in 🚀 |
Changes
This change adds a
goGeneratepostUpdateOption which enables runninggo generate ./...after a dependency update.Frequently when dependencies are brought in via tools or using vendored code, and those dependencies are used for go:generate instructions, we want to update the generated code after the dependency has been updated.
Adding this as a postUpdateCommand wasn't really satisfactory since that necessarily requires making a change in two spots (one in the allowed commands, one in the postUpdateCommand). The other reason this was undesirable was it would cause generate to run after every dependency update, regardless of language.
Context
Please select one of the below:
AI assistance disclosure
Did you use AI tools to create any part of this pull request?
Please select one option and, if yes, briefly describe how AI was used (e.g., code, tests, docs) and which tool(s) you used.
Documentation (please check one with an [x])
How I've tested my work (please select one)
I have verified these changes via:
The public repository: n/a (is private)