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_traffic_filter: Fix rule ordering bug #208

Merged

Conversation

marclop
Copy link
Contributor

@marclop marclop commented Dec 15, 2020

Description

Fixes a bug where multiple having a traffic filter with a few rules will
cause an infinite diff loop by changing the field from a List to a Set.
This consistently hashes the list using the same algorithm, which does
not happen when the field is of schema.TypeList.

Also adds a custom set hashing function for the traffic rules set, using
the source and optional description if found as hashing keys.

Last, changes the assertions to use TestCheckTypeSetElemNestedAttrs in
the acceptance tests, asserting the rule values without the position.

Related Issues

Closes #203

How Has This Been Tested?

Unit and acceptance tested, also tried with an example locally.

Types of Changes

  • Bug fix (non-breaking change which fixes an issue)

Fixes a bug where multiple having a traffic filter with a few rules will
cause an infinite diff loop by changing the field from a List to a Set.
This consistently hashes the list using the same algorithm, which does
not happen when the field is of `schema.TypeList`.

Adds unit and acceptance tests covering this case.

Signed-off-by: Marc Lopez <marc5.12@outlook.com>
@marclop marclop added bug Something isn't working Team:Delivery labels Dec 15, 2020
@marclop marclop added this to the v0.1.0 milestone Dec 15, 2020
@marclop marclop requested a review from a team as a code owner December 15, 2020 14:54
@marclop marclop self-assigned this Dec 15, 2020
Also adds a custom set hashing function for the traffic rules set, using
the source and optional description if found as hashing keys.

Last, changes the assertions to use `TestCheckTypeSetElemNestedAttrs` in
the acceptance tests, asserting the rule values without the position.

Signed-off-by: Marc Lopez <marc5.12@outlook.com>
@marclop
Copy link
Contributor Author

marclop commented Dec 16, 2020

The actual test that's touched here passes, there's a lot of noise currently in the acceptance tests with failures bubbling up from the platform due to ES instances failing to start in time:

From the Jenkins output:

11:31:03  --- PASS: TestAccDeploymentTrafficFilter_basic (12.63s)

From my laptop:

make testacc TEST_NAME=TestAccDeploymentTrafficFilter_basic
-> Running terraform acceptance tests...
=== RUN   TestAccDeploymentTrafficFilter_basic
=== PAUSE TestAccDeploymentTrafficFilter_basic
=== CONT  TestAccDeploymentTrafficFilter_basic
--- PASS: TestAccDeploymentTrafficFilter_basic (17.29s)
PASS
ok  	github.com/elastic/terraform-provider-ec/ec/acc	17.707s

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.

That looks like a tricky bug to catch! Thanks for fixing it :)

@marclop marclop merged commit b857eb7 into elastic:master Jan 5, 2021
@marclop marclop deleted the b/fix-ec_traffic_filte-rule-ordering branch January 5, 2021 05:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ec_deployment_traffic_filter rules order
2 participants