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

ec_deployment : Tags for deployment datasource #244

Merged
merged 10 commits into from
Feb 24, 2021

Conversation

neiljbrookes
Copy link
Contributor

@neiljbrookes neiljbrookes commented Feb 17, 2021

Description

This PR surafces any metadata tags associated with a deployment datasource.

  • Update schema.go to include "tags" in the ec_deployment schema
  • Update datasrouce.go (modelToState) to operate on any "tags" present in the model.
  • Add flatteners_tags.go (flattenTags) a function to flatten the model tags into a flat map.
  • Add flatteners_tags_test.go A unit test.
  • Update datasource_test.go. Adddition of tag flattening to the test.
  • Update ec_deployment.md. Includes "tags" as an attribute reference in the docs.

Related Issues

Closes : #182

Motivation and Context

  • Metadata tags as first class citizens.
  • Parity between resource and data_source.

How Has This Been Tested?

Unit and manual tests.

I have used this defintion to test.

resource "ec_deployment" "resource" {
  name = "resource"

  region                 = "aws-eu-west-1"
  version                = "7.10.1"
  deployment_template_id = "aws-cross-cluster-search-v2"

  elasticsearch {}
  kibana {}
  tags = {
    "foo"   = "bar",
    "bar"   = "baz"
  }
}

data "ec_deployment" "datasource" {
  id = ec_deployment.resource.id
}

output "debug" {
  description = "test"
  value = data.ec_deployment.datasource.tags
}

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 neiljbrookes added the enhancement New feature or request label Feb 17, 2021
@neiljbrookes neiljbrookes self-assigned this Feb 17, 2021
Copy link
Contributor

@karencfv karencfv left a comment

Choose a reason for hiding this comment

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

Thanks @neiljbrookes for opening up the PR! 🎉 🙇‍♀️

The code looks great! Apart from the docs comment, we should add this feature to the acceptance tests.

To modify the deployment datasource acc test you need to do two things:

  1. Add a tag to the test .tf file here
  2. Verify that the tag on the datasource is the same as the one in the resource here

The resource tags test can help as a guideline.

You can take a look at the resource tags test. It will be similar.

To run this test locally you would need to run make testacc TEST_NAME=TestAccDatasourceDeployment_basic.

Warning While this test is passing on master it's going through a "getting it fixed" state , this PR is the second part of getting it fixed. So you might want to wait for it to get merged so you can pull the changes otherwise you might also get some annoying merge conflicts 😄

docs/data-sources/ec_deployment.md Outdated Show resolved Hide resolved
@neiljbrookes
Copy link
Contributor Author

Rebased from master to get #242.
Force pushed.

Copy link
Contributor

@alaudazzi alaudazzi left a comment

Choose a reason for hiding this comment

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

LGTM for the doc change on ec_deployment.md.

@neiljbrookes
Copy link
Contributor Author

@karencfv I've made an attempt at some acceptance tests !

  • Updated TestAccDatasourceDeployment_basic to include tags.
  • Added TestAccDatasource_basic_tags and the associted TF defintion datasource_tags.tf.

Its quite simple, like me 😆.
Just a small "ec_deployment" resource with specific tags is created, then an "ec_deployment" datasource reads that data and the test ensure that the tags are as expected.

Tested OK for me

> 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 (184.86s)
PASS
ok      github.com/elastic/terraform-provider-ec/ec/acc 185.908s

The TestAccDatasourceDeployment_basic is still failing for me (usually with something that looks like https://github.com/elastic/cloud/issues/75870). Seems to still be affecting master, so I dont think it is somerthing that I broke !

@karencfv
Copy link
Contributor

Thanks a bunch for addressing my comments @neiljbrookes 🙇‍♀️

I tried running the TestAccDatasource_basic test with your changes locally several times and unfortunately I kept running into "plan failed due to unknown error" error messages from the API which are not related to your PR.

In the interest of moving your PR along I think the TestAccDatasourceDeployment_basic_tags test is enough, and we can get rid of changes to TestAccDatasourceDeployment_basic and datasource_deployment_basic.tf. Those can be added later when we can run the test 😄

@neiljbrookes
Copy link
Contributor Author

Thanks @karencfv .
I have removed the "tags" references from TestAccDatasourceDeployment_basic and datasource_deployment_basic.tf.

To clarify, I get successful tests on TestAccDatasource_basic_tags

> 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 (303.12s)
PASS
ok      github.com/elastic/terraform-provider-ec/ec/acc 304.208s

and faillure on TestAccDatasourceDeployment_basic

> make testacc TEST_NAME=TestAccDatasourceDeployment_basic
-> Running terraform acceptance tests...
=== RUN   TestAccDatasourceDeployment_basic
=== PAUSE TestAccDatasourceDeployment_basic
=== CONT  TestAccDatasourceDeployment_basic
    testing_new.go:63: Error running post-test destroy, there may be dangling resources: exit status 1
        
        Error: found deployment plan errors: 1 error occurred:
                * deployment [3171b9d74a5e4ee379440597c92be238] - [elasticsearch][43108bb53f2548f98c1b25b64eb538c8]: caught error: "plan failed due to unknown error"
        
        
        
        
--- FAIL: TestAccDatasourceDeployment_basic (475.01s)
FAIL
FAIL    github.com/elastic/terraform-provider-ec/ec/acc 476.064s
FAIL
make: *** [testacc] Error 1

Copy link
Contributor

@karencfv karencfv left a comment

Choose a reason for hiding this comment

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

Thanks for adding this feature @neiljbrookes 🎉 Everything looks great now

I have confirmed that the errors on the test are unrelated to this PR. We've had some ongoing issues with this suite particularly :(

@karencfv karencfv merged commit 6c425b8 into elastic:master Feb 24, 2021
@neiljbrookes
Copy link
Contributor Author

Thanks for all your help @karencfv.
My next challange is to add tag filtering for the ec_deployments datasource. I'm sure I'll be annoying you again shortly.

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.

ec_deployments: Filter by deployment tags
3 participants