Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(datasource/sites): support sites list #160

Merged
merged 6 commits into from
Apr 17, 2023

Conversation

Integralist
Copy link
Collaborator

@Integralist Integralist commented Apr 14, 2023

Fixes part of @sjparkinson issue reported here #111

terraform {
  required_providers {
    sigsci = {
      source  = "signalsciences/sigsci"
      version = "1.2.20"
    }
  }
}

resource "sigsci_site" "example1" {
  short_name   = "terraform_site1"
  display_name = "terraform test example site1"
}

resource "sigsci_site" "example2" {
  short_name   = "terraform_site2"
  display_name = "terraform test example site2"
}

resource "sigsci_site" "example3" {
  short_name   = "terraform_site3"
  display_name = "terraform test example site3"
}

data "sigsci_sites" "example" {
  depends_on = [sigsci_site.example1, sigsci_site.example2, sigsci_site.example3]
  filter     = "terraform test example site1" # checks against `name` and `display_name`
}

@Integralist Integralist added the enhancement New feature or request label Apr 14, 2023
@Integralist Integralist requested a review from shawnps April 14, 2023 16:35
@Integralist Integralist force-pushed the integralist/list-sites-datasource branch from 4d95f13 to 7b810e8 Compare April 14, 2023 16:39
Copy link
Collaborator Author

@Integralist Integralist left a comment

Choose a reason for hiding this comment

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

Comments for the reviewer...

go build -o terraform-provider-sigsci
build: clean
go build -o bin/terraform-provider-sigsci
@sh -c "'$(CURDIR)/scripts/generate-dev-overrides.sh'"
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This enables me to locally test the provider.

Comment on lines +51 to +55
"blacklist_uri": {
Type: schema.TypeString,
Computed: true,
Description: "Reference to the site's blacklist",
},
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

NOTE: The API has a blocklist field but it's not implemented in the go-sigsci API client.

return err
}

d.SetId("list_sites")
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Previously I was dynamically generating a dynamic ID but opted for a static ID as I don't imagine there will be multiple instances of the sigsci_sites data source. If we do think that is a possibility then we should make this dynamically generated (possibly using something like https://github.com/google/uuid to generate the ID).

if filter != "" {
for idx, site := range results {
if site["name"] == filter || site["display_name"] == filter {
return results[idx : idx+1]
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The implementation for the filter attribute is 'match only one', e.g. the first site that matches is returned.

If we want the filter to return multiple potential sites, then the implementation should be updated.

@@ -38,7 +38,7 @@ func expandStringArray(entries *schema.Set) []string {
}

func flattenDetections(detections []sigsci.Detection) []interface{} {
var detectionsSet = make([]interface{}, len(detections))
detectionsSet := make([]interface{}, len(detections))
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

My editor made a bunch of automated refactors.

@shawnps
Copy link
Collaborator

shawnps commented Apr 17, 2023

LGTM, thank you @Integralist!

@shawnps shawnps merged commit 2e4f25c into main Apr 17, 2023
@shawnps shawnps deleted the integralist/list-sites-datasource branch April 17, 2023 01:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants