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

Must be null related issues #352

Merged

Conversation

Kudbettin
Copy link
Member

@Kudbettin Kudbettin commented Aug 16, 2020

Then it must contain something

Then it must be null

  • This used to be a special case of Then its value condition match the "search regex" regex. This was problematic and not enough as documented by the listed issues.

  • Definition of what is considered to be null or not in this step is ambiguous and not documented well enough. (Once this PR is approved, documentation should be updated with the most recent state of the branch as well)

Current decisions is as follows:

  • Should it check if there are values within? (e.g. the current stash is not nothing)
    • Yes. For example, if there are no tags defined, the resource might still have an empty tags property. This should be considered as null (Used to be no)
  • Should it check for 'null' strings?
    • No. (Used to be yes)
    • This shouldn't be about the string but the value itself
    • nulls in the planfile/main.tf are converted to Nones in python. Having a 'null' is more likely to indicate the value is not null but happened to be named 'null' than otherwise.
  • Should it check for None?
    • For sure. This the bread and butter null value
  • Should it check for 'None' string
    • No. See the bullet for 'null' strings
  • Should it check for '' string
    • Yes, but I don't know why.
    • The old one checks it, so there should be some reason behind it
resource "aws_s3_bucket" "b" {
  bucket = "my-tf-test-bucket"
  tags = {
     SomeTag    = ""
  }
}
  • e.g. running the following steps on this resource would not pass:
Given I have resource that supports tags defined
Then it must have tags
And its value must not be null
* Should this stay as it is, or am I right to think it's incorrect to count this as null?
  • Should it check for '\x00'?
    • I have no idea what it does, just left it.
    • Probably similar to the bullet above.

Big Decision

  • When the step is "must not be null", should all inner values not be null or at least one non-null value is enough to consider the resource not null?
  • Could possibly add both options as new names, but it's pretty clutter-y
  • decision: condition_must not be null -> Ok if at least one element is not null
    condition must -> Fail if at least one element is not Null (i.e. Ok if all elements are null)
  • Is this a good decision?

Addresses #338, #343

@coveralls
Copy link

coveralls commented Aug 16, 2020

Pull Request Test Coverage Report for Build 1196

  • 5 of 26 (19.23%) changed or added relevant lines in 3 files are covered.
  • 6 unchanged lines in 1 file lost coverage.
  • Overall coverage increased (+0.006%) to 57.119%

Changes Missing Coverage Covered Lines Changed/Added Lines %
terraform_compliance/steps/steps.py 1 2 50.0%
terraform_compliance/steps/then/it_must_contain_something.py 0 3 0.0%
terraform_compliance/steps/then/its_value_condition_be_null.py 4 21 19.05%
Files with Coverage Reduction New Missed Lines %
terraform_compliance/common/defaults.py 6 85.0%
Totals Coverage Status
Change from base Build 1190: 0.006%
Covered Lines: 1344
Relevant Lines: 2353

💛 - Coveralls

@Kudbettin Kudbettin marked this pull request as ready for review August 16, 2020 18:30
Comment on lines +14 to +15
regex = r'{}'.format(u'(\x00|^$)$')
is_null = lambda x: x in ([], {}, None, Null) or (isinstance(x, str) and match.regex_match(regex, x))
Copy link
Member Author

Choose a reason for hiding this comment

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

Not checking for the string 'null' as it's converted to None automatically

is_null = lambda x: x in ([], {}, None, Null) or (isinstance(x, str) and match.regex_match(regex, x))

for resource in _step_obj.context.stash:
values = resource.get('values', {})
Copy link
Member Author

Choose a reason for hiding this comment

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

Smells legacy code. Is it even possible to not have values in a resource anymore?

Comment on lines +22 to +25
Error(_step_obj, '{} property in {} is considered to be {}. It is set to {}.'.format(_step_obj.context.property_name,
resource.get('address', _step_obj.context.name),
'Null' if values_is_null else 'not Null',
values))
Copy link
Member Author

Choose a reason for hiding this comment

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

For the error message:
can also put them both at the same time but feels a bit cluttery
e.g. _step_obj.context.name, resource.get('address', 'address could not be resolved')

@eerkunt eerkunt merged commit a4ce70d into terraform-compliance:master Aug 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants