Skip to content
New issue

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

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

Already on GitHub? # to your account

Fix "resource that supports" to accommodate "after_unknown" values #445

Merged
merged 6 commits into from
Jan 10, 2021

Conversation

Kudbettin
Copy link
Member

@Kudbettin Kudbettin commented Jan 7, 2021

Fixed an issue where the omission of after_unknown values were breaking the Given step resource that supports.

Addresses #438, #441

Please note, in this setup the regular Given works well:
using Given I have aws_rds_cluster defined with the other two steps:

  • passes with good kms_key_id
  • fails with bad kms_key_id (null)

Also note that kms_key_id is going to be an after_unknown value if and only if kms_key_id has a bad value (e.g. null)

So the Given I have resource that supports kms_key_id defined step is the culprit of this bug, as opposed to the omission of after_unknown values

Note: This function may be extended to capture 'after' values as well. That would require flattening the multi level
dictionaries in resource
'''
def remember_after_unknown(self, resource, after_unknown):
Copy link
Member

Choose a reason for hiding this comment

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

unit tests please :)

Copy link
Member Author

Choose a reason for hiding this comment

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

Added a couple :)


# if resource doesn't have type field, try to extract it from address
if not resource_type and 'address' in resource and resource['address']:
resource_type = resource.get('address').split('.')[0]
Copy link
Member

Choose a reason for hiding this comment

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

What if this is a module ?

Copy link
Member Author

Choose a reason for hiding this comment

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

True, then we would most likely need to use resource.get('address').split('.')[2] in that case.

However, I'm inclined to delete this clause all together. Is it possible to not have type in resource at first place?

Copy link
Member

Choose a reason for hiding this comment

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

Not sure, I think that depends on the provider really.

Copy link
Member Author

Choose a reason for hiding this comment

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

I made the change. However, we might run into a similar issue with the resource's address convention if provider doesn't add the type of the resource to the plan.

@ghost
Copy link

ghost commented Jan 8, 2021

Might want to add the feature file to the test dir? :)

@eerkunt
Copy link
Member

eerkunt commented Jan 8, 2021

Haha yes, I agree with @sleightsec here :)

@Kudbettin Kudbettin requested a review from eerkunt January 10, 2021 17:16
eerkunt
eerkunt previously approved these changes Jan 10, 2021
@eerkunt eerkunt merged commit c92fa89 into terraform-compliance:master Jan 10, 2021
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants