meilisearch / meilisearch-kubernetes

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

Check for out of sync manifest and add support for generating it automatically via a comment #65

Closed carlreid closed 3 years ago

carlreid commented 3 years ago

Attempting to tackle the manifest generation a little differently than before due to this.

To get what I think is the best middle ground, I've now made it so there's simply a check to see if the manifest is out of date, then fail if it is. When investigating, you will see the following message:

❌ Manifest update required. Comment on your PR with '@meilisearch sync-manifest' to update the manifest

As it says, comment on the PR with @meilisearch sync-manifest will then perform the job of updating the manifest and then adding a new commit to the pending PR. That way, we don't need to commit directly to main (due to branch protection) and then don't automatically commit to the working changes on PR (causing annoyance of rebasing/pulling for the creator).

I'm not sure if the @meilisearch sync-manifest is the best comment to use to trigger the job, so feel free to amend that to whatever you'd like 👍

Since the manifest is out of date, you should see the fail check already below. Feel free to comment with @meilisearch sync-manifest and see if it updates the PR and resolves the fail. I think it should work 🙈 Though maybe this change needs to be accepted first, before the job triggers.

Resolves: #12

carlreid commented 3 years ago

@eskombro Hopefully this removes the headaches around the other solution.

curquiza commented 3 years ago

Hello @carlreid, sorry for the delay. @eskombro is moving to another team in Meili, and we've just hired @alallema who takes the scope of @eskombro! 🙂 @eskombro is still in Meili so he will be able to take part in the (future) discussions in this repo, but less frequently! @alallema is new and it might take time before we come back to you here! Sorry for that, we try to be as reactive as possible!

Thank you again for all your involvement! ❤️

carlreid commented 3 years ago

@curquiza No bother, no rush on this one anyway 🙂

eskombro commented 3 years ago

@meilisearch sync-manifest

eskombro commented 3 years ago

Hi @carlreid

I am just passing by! I am no longer maintaining this repo permanently. will probably be around often, but @alallema will also be taking care of this repo! I wanted to thank you for all your contributions :)

I really like the new approach for this PR! The CI fail is great, and the idea of fixing it with the comment is amazing! I will let @alallema make the proper review!

Thanks and see you soon around :)

carlreid commented 3 years ago

@meilisearch sync-manifest

@eskombro Nice try 😆 I do think this needs to be merged first though before the workflow will execute.

I wanted to thank you for all your contributions

No problem 😊

I will let @alallema make the proper review!

No stress, I'd rather the efforts are focused on the new engine and such. Been keeping an eye on the transplant repo and what has being going on around it 👀 exciting!

alallema commented 3 years ago

bors try

bors[bot] commented 3 years ago

try

Build succeeded:

alallema commented 3 years ago

bors merge

bors[bot] commented 3 years ago

Timed out.

alallema commented 3 years ago

Bors merge

bors[bot] commented 3 years ago

Build succeeded: