-
Notifications
You must be signed in to change notification settings - Fork 16
Replace self-upgrade workflow with Renovate JSONata manager #636
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
This file was deleted.
This file was deleted.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3,7 +3,37 @@ | |
| extends: [ | ||
| 'github>cert-manager/renovate-config:default.json5', | ||
| ], | ||
| "customManagers": [ | ||
| { | ||
| "customType": "jsonata", | ||
| "fileFormat": "yaml", | ||
| "managerFilePatterns": ["klone.yaml"], | ||
| "matchStrings": [ | ||
| 'targets.*.{\ | ||
| "datasource": "git-refs",\ | ||
| "versioning": "git",\ | ||
| "depName": folder_name,\ | ||
| "packageName": repo_url,\ | ||
| "currentValue": repo_ref,\ | ||
| "currentDigest": repo_hash\ | ||
| }', | ||
| ] | ||
| } | ||
| ], | ||
| packageRules: [ | ||
| { | ||
| "groupName": 'Makefile Modules', | ||
| "matchManagers": ["custom.jsonata"], | ||
| "matchFileNames": ["klone.yaml"], | ||
| "postUpgradeTasks": { | ||
| "commands": [ | ||
| "make vendor-go", | ||
| "make generate-klone", | ||
| "make generate" | ||
|
Comment on lines
+30
to
+32
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. note: I think there's something interesting here I'd not thought enough about before.
I think the implication is that to use this self upgrade flow (maybe to use makefile modules at all?) you need at least the repository-base, tools and klone modules. Makes me think that our separation of modules isn't quite right - we have really important cross dependencies across these three core modules. I wonder if it would make sense to condense these modules down into a single (Not a blocker or anything because this is only for our consumption and we use those modules everywhere anyway - I'm just thinking out loud)
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There is actually a meaningful implication of this - the I don't think that's a blocker either, but it's worth calling out.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good point! Do we really need to have an exact Go version when running
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I had another look, and while I agree on your problem statement about module separation, I think this could be fixed in a follow-up PR I'll vote in favour of moving the vendor-go and unvendor-go targets to the repository-base module. And also merge the klone module into the repository-base module. WDYT, @inteon? But in the context of this PR, I propose to keep it as proposed. I just tested this PR in the website project, which is our project with less Go content, and it works. |
||
| ], | ||
| "executionMode": "branch", | ||
| } | ||
| }, | ||
| { | ||
| matchFileNames: [ | ||
| '**/go.mod', | ||
|
|
@@ -22,6 +52,5 @@ | |
| // Exclude files that are sourced from makefile-modules and shouldn't be upgraded in projects using makefile-modules. | ||
| 'make/_shared/**', | ||
| '.github/workflows/govulncheck.yaml', | ||
| '.github/workflows/make-self-upgrade.yaml', | ||
| ], | ||
| } | ||
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.
suggestion(claude): Found by AI, edited by me. I've not dug into checking if this is legit, so I'm not going to consider it a blocker or anything - but if you think it's legit + worth fixing, I'm happy to re-review next week!
Missing executionMode: 'branch'
The new packageRules entry for custom.jsonata is missing
executionMode: 'branch', which every other rule with postUpgradeTasks in the repo uses.Without it, Renovate defaults to per-update mode, meaning the three
makecommands run once per updated module entry. For a typical klone.yaml with several modules all pinned to the same hash those commands would run 7 times on a single branch. WithexecutionMode: 'branch', they run once for the whole branch.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.
Ok, this is interesting. I am going to POC this in the helm-tool project.
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.
cert-manager/helm-tool#279
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.
Ok, this works, and I have updated the PR to include
executionMode: 'branch'.