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

Filter ec_deployments datasource by tags #248

Merged
merged 10 commits into from
Mar 8, 2021

Conversation

neiljbrookes
Copy link
Contributor

@neiljbrookes neiljbrookes commented Feb 25, 2021

Description

This PR attempts to allow filtering by "tags" when using the ec_deployments datasource.

For example:

data "ec_deployments" "example" {

  elasticsearch {
    healthy = "true"
  }

  tags = {
      "foo" = "bar"
  }
}

output "test" {
  value = data.ec_deployments.example.deployments
}
> terraform output test                 
tolist([
  {
    "apm_resource_id" = "ba049aec4b0249818af25c7ef7543ba9"
    "deployment_id" = "10974c2d7136235d8fe88be9545a8d22"
    "elasticsearch_resource_id" = "fbeaf8097f1342dbb4cf25aea4310796"
    "enterprise_search_resource_id" = "dab52337b8414f57a4b5f40fb9c7ffb3"
    "kibana_resource_id" = "6f7a0a51fa584ace8be97845c50d6525"
  },
])

.... where 10974c2d7136235d8fe88be9545a8d22 is tagged with { "foo" = "bar"} and is elasticsearch-healthy !

Related Issues

Closes #182

Motivation and Context

How Has This Been Tested?

  • Unit test Test_expandFilters extended to include "tags".
  • Acceptance test TestAccDatasource_basic_tags extended to include ec_depoyments datasource filtering on tags.
> make unit
-> Running unit tests for terraform-provider-ec...
?       github.com/elastic/terraform-provider-ec        [no test files]
ok      github.com/elastic/terraform-provider-ec/ec     0.800s  coverage: 71.4% of statements
ok      github.com/elastic/terraform-provider-ec/ec/acc 1.159s  coverage: 4.7% of statements
ok      github.com/elastic/terraform-provider-ec/ec/ecdatasource/deploymentdatasource   1.068s  coverage: 89.6% of statements
ok      github.com/elastic/terraform-provider-ec/ec/ecdatasource/deploymentsdatasource  1.039s  coverage: 78.3% of statements
ok      github.com/elastic/terraform-provider-ec/ec/ecdatasource/stackdatasource        0.721s  coverage: 72.9% of statements
ok      github.com/elastic/terraform-provider-ec/ec/ecresource/deploymentresource       1.292s  coverage: 85.6% of statements
ok      github.com/elastic/terraform-provider-ec/ec/ecresource/extensionresource        0.788s  coverage: 78.4% of statements
ok      github.com/elastic/terraform-provider-ec/ec/ecresource/trafficfilterassocresource       0.770s  coverage: 66.7% of statements
ok      github.com/elastic/terraform-provider-ec/ec/ecresource/trafficfilterresource    0.705s  coverage: 72.9% of statements
ok      github.com/elastic/terraform-provider-ec/ec/internal/util       0.485s  coverage: 79.1% of statements
> make testacc TEST_NAME=TestAccDatasource_basic_tags
-> Running terraform acceptance tests...
=== RUN   TestAccDatasource_basic_tags
=== PAUSE TestAccDatasource_basic_tags
=== CONT  TestAccDatasource_basic_tags
--- PASS: TestAccDatasource_basic_tags (313.49s)
PASS
ok      github.com/elastic/terraform-provider-ec/ec/acc 314.426s

Types of Changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Refactoring (improves code quality but has no user-facing effect)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation

Readiness Checklist

  • My code follows the code style of this project
  • My change requires a change to the documentation
  • I have updated the documentation accordingly
  • I have added tests to cover my changes
  • All new and existing tests passed

@neiljbrookes
Copy link
Contributor Author

@karencfv (or @marclop perhaps ?!)
I've created this draft PR to seek some help.

Unit test Test_expandFilters keeps failing on me with

--- FAIL: Test_expandFilters (0.01s)
    --- FAIL: Test_expandFilters/parses_the_data_source (0.00s)
        expanders_test.go:84: 
                Error Trace:    expanders_test.go:84
                Error:          Not equal: 
                                expected: &models.SearchRequest{From:0, Query:(*models.QueryContainer)(0xc0000ae840), Size:0, Sort:[]interface {}(nil)}
                                actual  : &models.SearchRequest{From:0, Query:(*models.QueryContainer)(0xc0000aeea0), Size:0, Sort:[]interface {}(nil)}
                            
                                Diff:
                Test:           Test_expandFilters/parses_the_data_source
FAIL

The test asserts that the two SearchRequest's should be equal, and fails as they are not. The diff, however, is empty !

Essentiually I have added "tags" to the test model and the SearchRequest is returned from expandResourceFilters. I've attempted to extend newTestQuery to match this, but it keep failing (but the diff is empty).

HALP !

FYI. I've built and tested the binary....and it seems to work.

@neiljbrookes
Copy link
Contributor Author

@marclop I think that reflect.DeepEqual is doing something funny in the test.
I've committed ffd9afe which uses encoded JSON for the "is equal" assertion....and it works.

Bizarre.

@marclop
Copy link
Contributor

marclop commented Mar 1, 2021

@neiljbrookes I was trying this locally and I think it's due to the amount of nesting in these structures the assert.Equal isn't able to assert those properly, I'm OK +1 this PR, with the current approach since it's what we'll end up sending to the API.

I've modified your assertion so that it's easier to read should any errors occur on the test:

diff --git a/ec/ecdatasource/deploymentsdatasource/expanders_test.go b/ec/ecdatasource/deploymentsdatasource/expanders_test.go
index a3e8d98..3636f92 100644
--- a/ec/ecdatasource/deploymentsdatasource/expanders_test.go
+++ b/ec/ecdatasource/deploymentsdatasource/expanders_test.go
@@ -82,17 +82,17 @@ func Test_expandFilters(t *testing.T) {
 				assert.NoError(t, err)
 			}
 
-			jsonWant, err := json.Marshal(tt.want)
+			jsonWant, err := json.MarshalIndent(tt.want, "", "  ")
 			if err != nil {
 				t.Error("Unable to marshal wanted struct to JSON")
 			}
-			jsonGot, err := json.Marshal(got)
+
+			jsonGot, err := json.MarshalIndent(got, "", "  ")
 			if err != nil {
 				t.Error("Unable to marshal received struct to JSON")
 			}
 
-			assert.Equal(t, jsonWant, jsonGot)
-
+			assert.Equal(t, string(jsonWant), string(jsonGot))
 		})
 	}
 }

@neiljbrookes
Copy link
Contributor Author

Thanks @marclop.
I incorporated your changes in 5745736.

@marclop marclop marked this pull request as ready for review March 2, 2021 10:29
@marclop marclop requested a review from a team as a code owner March 2, 2021 10:29
@marclop
Copy link
Contributor

marclop commented Mar 2, 2021

oops I set the PR as ready for review. I think that was premature since the integration tests haven't been written yet.

@neiljbrookes neiljbrookes self-assigned this Mar 3, 2021
@marclop marclop changed the title Filter ec_deployments datasource by tags. Filter ec_deployments datasource by tags Mar 8, 2021
Copy link
Contributor

@marclop marclop left a comment

Choose a reason for hiding this comment

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

LGTM

@marclop
Copy link
Contributor

marclop commented Mar 8, 2021

acceptance failures are unrelated to the changes.

@marclop marclop merged commit 1fd5515 into elastic:master Mar 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ec_deployments: Filter by deployment tags
2 participants