search

cloudtools / troposphere

troposphere - Python library to create AWS CloudFormation descriptions
BSD 2-Clause "Simplified" License
4.92k stars 1.45k forks source link

Fix RDS Validations #2171

Closed JohnPreston closed 11 months ago

JohnPreston commented 11 months ago

The new dbinstance validations do not check properties types resulting in testing a string oracle against Ref for example. A lot of the tests should be ignored as soon as the DBClusterIdentifier is set, indicating that this is going to be an instance attached to Aurora DB Cluster.

Exception caught

        File "/home/john/.cache/pypoetry/virtualenvs/ecs-composex-6XqxN9Zj-py3.9/lib/python3.9/site-packages/troposphere/validators/rds.py", line 401, in validate_dbinstance
          and "oracle" in engine                                                                          
      TypeError: argument of type 'Ref' is not iterable
markpeek commented 11 months ago

@tnielsen2 could you take a look at this PR and approve given your recent PR? @JohnPreston is there a test that can be added to catch any regressions based on this change?

JohnPreston commented 11 months ago

@tnielsen2 could you take a look at this PR and approve given your recent PR? @JohnPreston is there a test that can be added to catch any regressions based on this change?

Hey @markpeek I will have a look and come up with something. I have my project which I created a branch for, to update troposphere version specifically, and it runs lots of tests, the only ones that failed were for RDS, hence how I found this.

I am guessing the validation test should cater for AWS HelperFn

tnielsen2 commented 11 months ago

This looks good to me. We don't run tests against validator logic, but the thought did cross my mind that having some in place for this would be great since there is a matrix of acceptable combinations with RDS specifically.

JohnPreston commented 11 months ago

Once merged, could we get a patch version released please? Right now this is just denying using any !Ref etc. for these properties.

markpeek commented 11 months ago

Published via Release 4.4.1