search

meilisearch / meilisearch-kubernetes

Meilisearch on Kubernetes Helm charts and manifests
https://www.meilisearch.com
MIT License
212 stars 59 forks source link

Update manifest on push to main #61

Closed carlreid closed 3 years ago

carlreid commented 3 years ago

This simply executes the helm template command and then performs a diff with git to see if there were any changes. If there are changes, then it will create a commit with those changes with the message [CI] Syncing Helm manifest.

I did initially have this run on a PR trigger, but it's very annoying if you have made changes to the chart since then you'd need to pull those changes down. Then if you were to follow up with more changes to the chart, it would result in another automated commit. Probably would add quite some noise to the commit history of [CI] Syncing Helm manifest 😄

The only "problem" I have found with this approach, is that a lot of tags are added to the manifests that are completely unnecessary and pollute them

In response to this, I assume this is in relation to helm.sh/chart and app.kubernetes.io/managed-by. I'd disagree that they're unnecessary as the app.kubernetes.io/managed-by is a recommended label and so is the helm.sh/chart as stated here. So I think it's a good thing that they're being included? Maybe there's good reasons you guys don't want them in though?

Example run of the action. Example of the automated commit (can also check the commit history) Resolves: #12

carlreid commented 3 years ago

the goal of having manifests is to be able to use them directly, without Helm (or depending on it). These manifests should be able to be applied directly on a k8s cluster, and this is why I feel that in this case, those tags are polluting.

I did think after, why you may want them and that's the conclusion I came to. Anyhow, I agree that they're really odd to have if you didn't use Helm to actually deploy it. I've updated the PR now and introduced a grep between the template and output file that will skip over lines that include helm.sh/chart: or app.kubernetes.io/managed-by: before writing the output. You can see the updated result here.

eskombro commented 3 years ago

That is perfect @carlreid !!!

Amazing, proceeding to merge, thanks a lot!

bors try

bors[bot] commented 3 years ago

try

Build succeeded:

eskombro commented 3 years ago

Bors merge

bors[bot] commented 3 years ago

Build succeeded:

carlreid commented 3 years ago

You're not authorized to push to this branch

@eskombro Hmm... You guys may have to help out here. Guess since you have bors managing the merges then it would need write permissions.

eskombro commented 3 years ago

Oh. I forgot about this 'little' detail: main is a protected branch and the CI is not able to push on it with the GH token. This is complex, since allowing push on main with GH token would allow any user with write access to push on main, which makes the branch protection useless...

Any idea on how to find a workaround for this problem?

Will discuss this with @curquiza tomorrow, since she handles the rights on the repo!