Open tasdomas opened 1 year ago
We could set spot
to Float64Var
instead of BoolVar
so it's consistent with the legacy Terraform schema.
However, asking users to pass --spot=0
in order to enable automatic spot pricing seems rather counterintuitive to me. I'd even ask myself whether allowing specifying a fixed spot price is a good idea. 🤔
whether allowing specifying a fixed spot price is a good idea
Do all clouds guarantee default(auto) spot pricing
< on-demand
?
I think the root cause of this is using the same binary for the CLI tool, for instance shutdown in provisioned machines and as a terraform provider plugin. I don't think there would be any harm in separating the three. The only blocking issue would be finding proper names for the three..
Do all clouds guarantee
default(auto) spot pricing
<on-demand
?
aws
az
gcp
Not applicable
I think the root cause[^1] of this is using the same binary for the CLI tool, for instance shutdown in provisioned machines and as a terraform provider plugin.
Using the same binary as a Terraform provider and as a command line tool is definitely a bizarre choice. Still, it's not directly related to the float64
versus bool
conundrum; API should look the same everywhere if it makes sense.
I don't think there would be any harm in separating the three.
The devil is in the details, but yes, task
can be safely extracted as a module without changing a single byte of the code apart from the name.
The only blocking issue would be finding proper names for the three.
Not sure if it's “the only” issue, but it's probably the biggest one.
[^1]: Note that root causes are rarely the root causes you're looking for. 😅
so something like this?
on_demand: bool=True
task
: spot: float=-1
on_demand: bool=True
Given that spot
is a widely used term, I'd consider using it instead of more creative alternatives like !on_demand
🤔
I thought GCP uses s/spot/preemptible/
and s/on.demand/standard/
:)
Google Cloud followed the spot
naming trend recently. 😅
Anyway the point of renaming is mostly to retain some backward-compatibility?
The alternative -- spot: bool=False
-- will silently break existing user workflows that have spot: float
(presumably the float will get cast to a bool in the most undesirable way?)
the point of renaming is mostly to retain some backward-compatibility
Isn't it a bit too early to get started with “perfect backwards compatibility” on this project?
However, asking users to pass --spot=0 in order to enable automatic spot pricing seems rather counterintuitive to me.
Was your idea 😁 machine
and runner
have that term as spot
and spot_price
@0x2b3bfa0
Guilty as charged! 😅 Exposing two separate options didn't seem to me a good option either.
I wonder if we should just expose --spot=<boolean>
and leave pricing to cloud providers. Who wants to set it manually anyway?[^1]
[^1]: Not a haphazard assumption anymore, let's check telemetry data.
I found this while testing out basic example in the README
If a user were to set
spot
to anything other than0
, the task will fail to start:An example
main.tf
that reproduces this (the only line changed isspot = 0.05
):This is caused by multiple decisions at multiple levels:
tpi
binary attempts to interpret amain.tf
file for parameterstpi
binary expectsspot
to parse as a booleanThere are several ways to address this, including updating the basic terraform configuration example, but I think the way
tpi
parses thespot
field needs to be changed.