-
Notifications
You must be signed in to change notification settings - Fork 152
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
Ignore after_unknown resources #417
Ignore after_unknown resources #417
Conversation
Note: Please see issue #408 for help and guidance on ability to filter on only new @Kudbettin. Really great findings and explaination! I commented on my issue #416 , figured I would comment here as well. The ability to account for the absence of a parameter below the root block values level as well as an incorrect value are both key for us. Piping in default values can create a host of unintended false negatives on our sude. We are curious as to where you are piping in the default for |
@Kudbettin I realize this project is a labor of spare time, and word on when this could get implemented? This issue and the ability to filter on only NEW (create) and (update), one, the other and both to only address fixes on new infrastructure , and temper the fails on existing resources that would be resource destroying ( as an option flag) are 2 blockers for our proposed implementation. |
I did some digging and it looks like you are piping in the |
Kinda duplicated by #422. Closing this issue. :) |
Ignore after_unknown resources
This PR removes adding "known after apply" values to the resources during mounting. I'll be discussing why and what could be the alternative solutions.
TLDR
Example of "known after apply" values:
Problem: We're including the "known after apply" values inside the stash with their default values. In #410, the default value of
encrypted
istrue
. Such cases are problematic.Solution: set them all to a unique default value to prevent confusion
This solution was implemented in here
How it looked in stash before the change:
Note that dict_merge also didn't merge dictionaries correctly, so that was fixed as well.
In this branch, we'll simply not incorporate "after_unknown" values of plan to our resources
Problem: We're including the "known after apply" values inside the stash (regardless of their value). Some tests use them incorrectly.
Example: test_dynamodb_kms_key_problem is a meaningless test.
And it must contain kms_key_arn
step will always pass regardless of providing a kms_key_arn within theserver_side_encryption
(in terraform file) or not.Solution: None except not writing tests that rely on "known after apply" values. (See below)
Problem: We're including "known after apply" values only on certain levels.
dict_merge
doesn't make sense. see test_dynamodb_kms_key_problem and test_issue-279Just to document why dict_merge didn't work (or this behavior was possibly intended).
kms_key_arn is added to the stash while health_check_type is not added. They both belong to "after_unknown" part of the plan. Why this's a problem? Just overall inconsistency in the tool and potential to write meaningless tests.
test_issue-279 passes because "known after apply" value is not added to the stash since it's in the "root level". In test_dynamodb_kms_key_problem, a "known after apply" value is added to the stash, making the test pass.
Solution: No need, already stopped using
dict_merge
Important
Problem: There is no way to test known after apply values. (e.g. actually test for kms_key_arn)
It seems having
vs
has not impact on the plan.
The only way of checking for
kms_key_arn
that I can think of is either applying, or checking main.tfchecking main.tf
How are we going to do this?
kms_key_arn = aws_kms_key.test.arn
for instance)checking after applying
Another way of saying, no way to test this beforehand. Would testing things after "apply" be useful?
doing nothing
Accepting that we can't write those kinds of tests. (Whether this PR merges or not)
Bad tests have to fix themselves
For now, tests that rely on "known after apply" values should fix themselves (they are likely not working at the moment). I commented out the failing line in one of the integration tests, since it was a "bad test" as well.
Addresses #410, #416
See other branch for more complicated and arguably worse solution