equinix / terraform-provider-metal

DEPRECATED Equinix Metal standalone TF provider. Use the unified Equinix Terraform Provider.
https://registry.terraform.io/providers/equinix/equinix/latest/docs/guides/migration_guide_equinix_metal
Mozilla Public License 2.0
14 stars 11 forks source link

Datasource metal_plans #194

Closed t0mk closed 2 years ago

t0mk commented 3 years ago

This is a new datasource for querying plans, fixing equinix/terraform-provider-equinix#169 This is just a draft, for discussion.

It's done in the digitalocean style, like https://registry.terraform.io/providers/digitalocean/digitalocean/latest/docs/data-sources/images

..with the filter blocks. The functionality allowes more than filters, it can also sort, and match by RE.

@displague What do you think? I think that if we want ot accumulate filters, we should do it in a clean and structured way. And here we can take advantage of an existing clean and structured way.

I basically took the filtering logic code from DO provider. It's under the same licences (MPL2), and after brief research, I think there's nothing we should explicitly do in order to have this legit. I also didn't find a way how to properly acknowledge this.

Signed-off-by: Tomas Karasek tom.to.the.k@gmail.com

t0mk commented 3 years ago

I added less_than and more_than keywords to match_by field in the filter block. It can be used to filter floating point values (I should test it for int fields), giving possibility to select only plans under some hourly price threshold.

Following hcl will select device (not storage) plans which are under 1$ per hour, are available in metro ty and sort it by the hourly price ascending.

data "metal_plans" "test" {
    sort {
        key = "pricing_hour"
        direction = "asc"
    }
    filter {
        key = "pricing_hour"
        values = [1]
        match_by = "less_than"
    }
    filter {
        key = "line"
        values = ["baremetal"]
    }
    filter {
        key = "available_in_metros"
        values = ["da"]
    }
}

output "test" {
    value = data.metal_plans.test
}
codecov-commenter commented 3 years ago

Codecov Report

Merging #194 (56b19c3) into main (47a745a) will increase coverage by 1.51%. The diff coverage is 95.17%.

@@            Coverage Diff             @@
##             main     equinix/terraform-provider-metal#194      +/-   ##
==========================================
+ Coverage   75.38%   76.89%   +1.51%     
==========================================
  Files          50       51       +1     
  Lines        6659     6891     +232     
==========================================
+ Hits         5020     5299     +279     
+ Misses       1414     1354      -60     
- Partials      225      238      +13     
Impacted Files Coverage Δ
metal/resource_metal_organization.go 83.18% <92.24%> (+34.90%) :arrow_up:
metal/datasource_metal_organization.go 75.73% <95.55%> (+29.06%) :arrow_up:
metal/datasource_metal_plans.go 97.27% <97.27%> (ø)
metal/datasource_metal_connection.go 98.67% <100.00%> (+0.01%) :arrow_up:
metal/provider.go 100.00% <100.00%> (ø)
metal/resource_metal_virtual_circuit.go 77.27% <100.00%> (+0.32%) :arrow_up:
metal/utils.go 76.08% <100.00%> (+3.58%) :arrow_up:
metal/errors.go 63.30% <0.00%> (-15.60%) :arrow_down:
metal/resource_metal_device.go 64.55% <0.00%> (-2.59%) :arrow_down:
metal/helpers_device.go 82.10% <0.00%> (-0.53%) :arrow_down:
... and 4 more

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 47a745a...56b19c3. Read the comment docs.

displague commented 3 years ago

We may want to reserve filter for the server side filtering, https://github.com/equinix/terraform-provider-equinix/issues/165. While the API spec does not use the word filter, packngo does.

Should we offer client and server filters? filter vs something-filter?

Or perhaps we use client filters, as you've provided, and provide a server-side = true option? In that case, I assume we can only allow 'exact' comparisons. I suspect that behavior may not be consistent from field to field.


filter {
  name = "name"
  value = "foo"
  type = "exact" # is this default?
  api = true # maybe a better name than server-side
}
t0mk commented 2 years ago

@displague I came back to this. I see that the filtering functionality from equinix/terraform-provider-equinix#165 clashes with the provider-logic filters.

I think we can't have the API filter and provider logic filter in the same sub-schema. the provider logic filtering acts on the attributes of the datasource, and they might be different from the API resource attributes.

I am not sure if using the API filters have any sense here or in any datasoruce ino the provider. The provider logic filtering is better, it offers

Arguably, using API search can make queries faster for customers that have tens or hundreds of resources.

For the API search, I'd suggest sth like

    api_filter {
        key = "available_in_metros"
        value = "da" // singular
    }
displague commented 2 years ago

We should add the reservation_pricing field to packngo and include that in Terraform. (cc @cprivitere )

ocobles commented 2 years ago

I want to take this back to improve the acceptance tests as @displague mentioned https://github.com/equinix/terraform-provider-equinix/issues/169

In general, I agree with the above. I would keep client/server filters separate. Adding this extra functionality not available in the API, should not be a problem as long as the difference is well described in the provider documentation. I second the idea of setting "api_filter" for server-side which explicitly describes that the keys must be the API payload args e.g. "available_in","specs.cpus.count",etc.

Some planned changes:

Question @displague: should we use "attribute" instead of "key" ? I want to reinforce that it is provider logic and not the API

filter { key = "pricing_hour" attribute = "pricing_hour" values = [1] match_by = "less_than" }

displague commented 2 years ago

replace schema field "available_in" with "available_in_facilities"

I think this would be:

filter {
  attribute = "facilities" #whatever the field name is "facility"?
  values    = ["da11"]
  match_by  = "in"
}

Perhaps match_by should default to in which is effectively equals when a single value is specified.

should we use "attribute" instead of "key" ? I want to reinforce that it is provider logic and not the API

attribute sounds fine. We could possibly use the field name to determine if this is a server-side or client-side filter:

filter {
  attribute = "terraform_name"
  value     = "foo"
}

filter {
  name      = "api_name"
  value     = "foo"
}

name and attribute would be exclusive, one of which would be required.

Value vs Values could be used for single vs multiple value operations. Again, mutually exclusive and required.

filter {
  value = "1"
}

filter {
  values = ["1", "2"]
}

For operations like less_than, a single value should be sufficient and we could return an error if more than one value is provided, but it may be easier to suggest that value be used (and not values) with certain operators (match_bys).

Would we have to accept all values as strings and convert numerics and booleans to strings?

displague commented 2 years ago

I think this would be:

filter {
 attribute = "facilities" #whatever the field name is "facility"?
 values    = ["da11"]
 match_by  = "in"
}

I didn't realize the field name was available_in. It makes sense for available_in to be the attribute in this case.

ocobles commented 2 years ago