Open pnordh opened 2 years ago
Hi there,
Help us by making a minimal reproduction repository.
Before we can start work on your issue we first need to know exactly what's causing the current behavior. A minimal reproduction helps us with this.
To get started, please read our guide on creating a minimal reproduction to understand what is needed.
We may close the issue if you (or someone else) have not provided a minimal reproduction within two weeks. If you need more time, or are stuck, please ask for help or more time in a comment.
Good luck,
The Renovate team
Sounds like this would fit into our "artifacts" concept, as we already support vendored components in other managers too. A reproduction is needed here so that we can see an example, and particularly an example of a current PR which is incomplete.
Hi, you can find a reproduction here https://github.com/pnordh/renovate-14137-reproduction with the pnordh/renovate-14137-reproduction#1 PR as an example of an "incomplete" PR.
Thanks, reproduction forked to https://github.com/renovate-reproductions/renovate-14137-reproduction
Looping in @andrein (who built the original feature in https://github.com/renovatebot/renovate/pull/12628), in case you have ideas (or want to help out with) a solution for this. 😊
@MPV I'll find some time to look at this next week, thanks for tagging me :)
Hi,
I've looked at the reproduction (thanks @pnordh!) and I think this can be handled by renovate.
The only caveat I see is mentioned here
kustomize is a YAML manipulator. It's not a manager of a cache of things downloaded from the internet.
This could be easily fixed by removing the charts/<dependency>
directory and running helm pull
as described here
$helmCommand pull \
--untar \
--untardir $DEMO_HOME/base/charts \
--repo https://itzg.github.io/minecraft-server-charts \
--version 3.1.3 \
minecraft
@rarkins @MPV how do you feel about this solution? Is there another manager I could draw inspiration from to implement this?
Can someone give an example of what a complete PR would look like? Then I am better placed to comment
@andrein Thanks for taking a look!
That sentence in the Kustomize docs means “if there is already a chart downloaded, it won’t try again”.
However, if one were to remove the downloaded chart, running Kustomize again, it will download it again.
So I think running helm
for downloading shouldn’t be necessary as Kustomize
can do it (as long as the “cached” chart in the repo has been deleted beforehand).
I’m thinking:
<thedirectory>/charts/*.tar.gz
files. kustomize build <thedirectory>
.Thoughts?
@rarkins Here's an example of a complete (expected) PR: https://github.com/pnordh/renovate-14137-reproduction/pull/3
...compared to the already shown actual behavior in: https://github.com/pnordh/renovate-14137-reproduction/pull/1
Definitely looks similar to the gomod
manager support for "vendored" modules. Is charts/
reserved for this purpose?
While charts/
is the common convention it's not reserved for it. Users of the helm chart inflator also have the possibility of using another path, see https://github.com/kubernetes-sigs/kustomize/blob/master/api/types/helmchartargs.go#L7-L19
Sorry I don't understand exactly how that file works. If the user changes the default path would we see it in a file in the repo we can parse?
@rarkins yes, we can read that field from kustomize.yaml
, with a default of charts
if it's not present.
See this example.
helmGlobals:
chartHome: charts
@andrein & @rarkins Thanks for taking a look at this. Let us know if there's anything else needed to move forward on this.
I'm not sure who's meant to be moving it forward, but I don't see any impediments
I'm sorry, I haven't found the time to fix this (yet). I hope I'll get a chance next week.
In the meanwhile, I can demo the workaround we have working somewhat okay in our repos; a GitHub actions workflow that looks like this:
name: Renovate Chart Refresh
on:
pull_request:
paths:
- "**/kustomization.yaml"
jobs:
renovate-chart-refresh:
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v3
with:
fetch-depth: 0
ref: ${{ github.head_ref }}
token: ${{ secrets.GITHUB_TOKEN }}
- run: |
kustomize_dirs=`git diff --name-only origin/master | grep 'kustomization.yaml' | sed 's|/kustomization.yaml||'`
for x in $kustomize_dirs; do
rm -rf "$x/charts"
kustomize build --enable-helm "$x" > /dev/null
done
- uses: stefanzweifel/git-auto-commit-action@v4
with:
commit_message: "chore(deps): update kustomize helm chart vendoring"
@andrein Any updates? Sorry for the poke. Really happy you're interested in contributing this (and hoping you're still open for it). ❤️
Just wanted to share additional interest in this. I see that it's been a while since the last update.
Friendly reminder @andrein / @rarkins 🙏
A drawback of the workaround I mentioned above is that Renovate detects it as an external change, so it stops updating such PRs with any new upgrades, i.e. like this: 😥
I have unassigned @andrein as he seems busy. Please don't add any further comments chasing.
@MPV you need to look into gitIgnoredAuthors
What would you like Renovate to be able to do?
Right now when the kustomize manager resolves an upgrade for a dependency of the helmchart deptype the kustomization.yaml is upgraded but the chart dependency is not inflated which means that the resulting Renovate PR is incomplete for repositories where the inflated chart is vendored (as per kustomizes best practice: https://github.com/kubernetes-sigs/kustomize/blob/master/examples/chart.md#best-practice).
If you have any ideas on how this should be implemented, please tell us here.
Renovate would likely need to first be able to determine if the chart is vendored, first because it likely needs to pull out if the chart is not vendored, but also in order to delete it before running
kustomize build --enable-helm
on the upgraded kustomization.yaml as per https://github.com/kubernetes-sigs/kustomize/blob/master/examples/chart.md#how-does-the-pull-work (i.e kustomize doesn't pull down the chart if it's already inflated regardless of version).Is this a feature you are interested in implementing yourself?
No