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 then it must contain #327

Merged

Conversation

Kudbettin
Copy link
Member

Changed Then it must contain to properly drill down and split into multiple resources if need be

Example stash:

[
    {
        "address": "azurerm_storage_account.example",
        "values": [
            {
                "bypass": [
                    "AzureServices",
                    "Logging",
                    "Metrics"
                ],
                "default_action": "Deny",
                "ip_rules": [
                    "100.1.1.123/32"
                ]
            },
            {
                "bypass": [
                    false,
                    false,
                    false
                ],
                "ip_rules": [
                    false
                ],
                "virtual_network_subnet_ids": true
            }
        ],
        "type": "azurerm_storage_account"
    }
]

If we run Then it must contain bypass the new stash should look like

[
    {
        "address": "azurerm_storage_account.example",
        "values": [
            "AzureServices",
            "Logging",
            "Metrics"
        ],
        "type": "azurerm_storage_account"
    },
    {
        "address": "azurerm_storage_account.example",
        "values": [
            false,
            false,
            false
        ],
        "type": "azurerm_storage_account"
    }
]

Two new resources were put into stash as there were two instances of bypass in the previous stash.

This introduces some problems to previous issues such as #285, where the example stash is coming from and azurerm adds the second bypass to the plan. Drilling down to bypass will expose the second bypass instance to the following steps.

I think issues like this should adjust their scenarios since the old behavior was practically faulty.

An alternative scenario for the issue could be:

@noskip_at_line_7
    Scenario: Ensure 'Trusted Microsoft Services' is enabled for Storage Account access
        Given I have azurerm_storage_account defined
        Then it must contain network_rules
        And it must contain bypass
        When it has AzureServices # line 7

In this scenario at least one bypass should contain AzureServices

or

	@noskip_at_line_15
	Scenario: Ensure 'Trusted Microsoft Services' is enabled for Storage Account access 2
		Given I have azurerm_storage_account defined
		Then it must contain network_rules
		And it must contain bypass
		When it does not have False  # line 15
		Then any of its values must match the "(^AzureServices$)" regex

where the scenario filters the unwanted bypass instances before moving to the THEN step. (A third alternative would be entirely removing line 15 and the @noskip tag)

(Tests of this issue was also modified to accommodate the changes)

Notes: Additional filtering needed, to achieve the desired state. (e.g. filter out the bypass with [False, False, False])

Changed When it contains to accomodate singular values within when
No comments here

Note: There were a bit more comments than usual, please let me know if they should be changed/removed

This issue partly addresses #307

@coveralls
Copy link

coveralls commented Jul 15, 2020

Pull Request Test Coverage Report for Build 1138

  • 11 of 66 (16.67%) changed or added relevant lines in 2 files are covered.
  • 8 unchanged lines in 2 files lost coverage.
  • Overall coverage decreased (-0.1%) to 58.981%

Changes Missing Coverage Covered Lines Changed/Added Lines %
terraform_compliance/steps/when/it_contains_something.py 0 3 0.0%
terraform_compliance/steps/then/it_must_contain_something.py 11 63 17.46%
Files with Coverage Reduction New Missed Lines %
terraform_compliance/steps/then/it_must_contain_something.py 2 19.71%
terraform_compliance/common/defaults.py 6 84.21%
Totals Coverage Status
Change from base Build 1135: -0.1%
Covered Lines: 1297
Relevant Lines: 2199

💛 - Coveralls

# 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.

3 participants