iterative / terraform-provider-iterative

☁️ Terraform plugin for machine learning workloads: spot instance recovery & auto-termination | AWS, GCP, Azure, Kubernetes
https://registry.terraform.io/providers/iterative/iterative/latest/docs
Apache License 2.0
287 stars 27 forks source link

Use `region` field as comma-delimited node selector for k8s #657

Closed sjawhar closed 1 year ago

sjawhar commented 1 year ago
0x2b3bfa0 commented 1 year ago

Hacky?

Yes, cough, cough, yes: https://github.com/iterative/terraform-provider-iterative/issues/412#issuecomment-1159729184. Nevertheless, sounds useful to me.

dacbd commented 1 year ago

willing to add some documentation snippets?

sjawhar commented 1 year ago

Yes, cough, cough, yes: #412 (comment). Nevertheless, sounds useful to me.

I got the impression from the convo on my last PR that we didn't want to add cloud-specific options, but I'm happy to decline this PR and do it that way if preferred :smile:

0x2b3bfa0 commented 1 year ago

We have some mixed opinions on the topic, but this proposal sounds good to me. It's a stretch of the “region” concept, but at least it adds some support for k8s node selectors instead of outright ignoring the region field.

sjawhar commented 1 year ago

It looks like the smoke test failed because of "Quota Exceeded"

omesser commented 1 year ago

It's a stretch of the “region” concept, but at least it adds some support for k8s node selectors instead of outright ignoring the region field.

Yes indeed :) I have no strong objection to merging this (even if it becomes a small tech debt..), but afterwards - let's please discuss proper support for k8s specific configuration (it's not a cloud provider), cc @iterative/cml I am not convinced we're doing anything other than delaying the inevitable and confusing users with mixing up of terminologies 🥲

0x2b3bfa0 commented 1 year ago

Also passed the Kubernetes smoke test :shipit: