Skip to content

Conversation

@tarrow
Copy link
Contributor

@tarrow tarrow commented Feb 5, 2025

Describe the changes

This stops using helm and uses plain kubectl to deploy the changes since after argo there is now no helm owned releases in the local cluster

This is what I need help with

Link to Phabricator

Prior discussion

You can cherry-pick this branch on to the latest main to see if this workaround will allow you to successfully skaffold the api.

Copy link
Member

@outdooracorn outdooracorn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for creating this @tarrow! I tested this out and it works. 🎉

However, I think it needs a little more work before merging. 🧑‍🏭
I've also left some questions to aid my understanding. 🤓

before:
- host:
command: [ "./helmfile-values", "-e", "local", "-r", "api", "-n" ]
kubectl: {}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at the skaffold docs it seems like a deploy.kubectl will by default have a manifests value of - "k8s/*.yaml"? And this is how it picks up the k8s/api*.yaml files?

Rather than relying on this default magic, how about specifying it here, so readers of the file are likely to follow without having to find the correct parts of the Skaffold docs? Or maybe even are more restricted wildcard of k8s/api-*.yaml?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suspect I hadn't read the skaffold docs and maybe reverse engineered the missing files to be in place! I followed your link and didn't actually manage to see the manifests docs. I managed to get as far as here: https://skaffold.dev/docs/deployers/kubectl/ which doesn't really have that much helpful stuff

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Urgh! The stupid skaffold anchor link didn't work. If you go to https://skaffold.dev/docs/references/yaml/?version=v2beta23 (same link as above but without the anchor) and then add the URL anchor #deploy-kubectl after the page has loaded, it should hopefully take you to the correct part of the yaml reference that seems to indicate that - "k8s/*.yaml" is the default for kubectl.manifests (failing that Ctrl + F for kubectl:).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How are these k8s/api-*.yaml files being kept in sync with the helmfile/argo values files? I'm also not a fan of having to modify the same values in multiple files (e.g. to swap from using mw1.39 to mw1.43 DBs).

How did you create these files in the first place? Maybe we can create a script that gets run before skaffold to create these files from the source of truth (currently the helmfile values files)? If they become derived files, do they even need to be checked into the repo?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 i think this will bite us if we just duplicate here

not sure how tom did it but this command gives something similar: helmfile -e local template -l name=api -f only-for-argo-value-generation.yaml

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, in general I don't think we should merge this patch :P. I think this is a very rough starting point for trying to use skaffold without helm.

I think your pre-run script also sounds smart and we could do that but I'd kinda hope we could figure out a more skaffold native way

@outdooracorn outdooracorn force-pushed the skaffold4API branch 2 times, most recently from 1840592 to 13e9549 Compare November 12, 2025 13:59
@outdooracorn
Copy link
Member

@tarrow pointed out that I had made it hard to cherry-pick this PR by merging main into this PR, rather than rebasing and force-pushing. I've now addressed that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants