techno-tim / k3s-ansible

The easiest way to bootstrap a self-hosted High Availability Kubernetes cluster. A fully automated HA k3s etcd install with kube-vip, MetalLB, and more. Build. Destroy. Repeat.

https://technotim.live/posts/k3s-etcd-ansible/

Apache License 2.0

2.41k stars 1.05k forks source link

➕ Add default values to roles #509

Closed bdsoha closed 2 months ago

bdsoha commented 5 months ago

Proposed Changes

Add default values to roles to refrain from missing values in the all.yml file.
Allow modular use of roles when consumed as a collection.
Add metadata to all roles with documented arguments.

Checklist

[ ] Tested locally
[ ] Ran site.yml playbook
[ ] Ran reset.yml playbook
[ ] Did not add any unnecessary changes
[ ] Ran pre-commit install at least once before committing
[ ] 🚀

bdsoha commented 5 months ago

@timothystewart6 It would be great to receive your input on whether or not such a PR is in the roadmap for this project. If so, I will continue the work for all other roles.

bdsoha commented 5 months ago

@timothystewart6 Did you have a chance to look at this?

timothystewart6 commented 5 months ago

@timothystewart6 Did you have a chance to look at this?

Hi! Yes, this sounds great as long as we don't break how it currently works. Unfortunately CI isn't working right now but I hope to have it working soon so that the tests will run!

Thanks! Tim

timothystewart6 commented 4 months ago

Just let me know when this is ready!

bdsoha commented 4 months ago

No problem, I will change the PR status from draft to ready.

bdsoha commented 4 months ago

@timothystewart6 Ready for review!

timothystewart6 commented 3 months ago

@bdsoha it seems this is failing CI

see logs: https://github.com/techno-tim/k3s-ansible/actions/runs/10134226628/job/28020331172

seems like you are mixing snake_case and camelCase. I would just make them all snake_case and one small conflict after all the merges

bdsoha commented 3 months ago

@timothystewart6 I didn't choose the variable names, just used the existing ones (which might cause regression failures for users of the playbook). Therefore, I added ignore statements to the relevant lines.

pre-commit is passing locally for me.

timothystewart6 commented 3 months ago

Well they were only in a template before now they are set as defaults and used elsewhere. I think you maybe have missed some ignores. There are also conflicts too. Although it does work locally, you might be on a different version? It does need to pass CI since it's the source of truth.

bdsoha commented 3 months ago

@timothystewart6 You are referring to a CI run from a previous commit (before I added the ignore statements). There is no CI run for the most recent commit which resolves the issues.

The most recent commit is "requires approval from a maintainer".

timothystewart6 commented 3 months ago

@bdsoha sorry! Good call! There are still conflict though however

bdsoha commented 3 months ago

@timothystewart6 Done!

timothystewart6 commented 3 months ago

Unfortunately it's not passing CI