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

ApiGateway resources do not match documentation #550

Open
benbridts opened this issue Aug 18, 2016 · 7 comments · May be fixed by #749
Open

ApiGateway resources do not match documentation #550

benbridts opened this issue Aug 18, 2016 · 7 comments · May be fixed by #749

Comments

@benbridts
Copy link
Contributor

benbridts commented Aug 18, 2016

I don't have time right now to fix it myself, so I'm logging it here.

This is the current code, with in comment what the docs say about required (http://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-resource-apigateway-method.html)

# Left out all methods that are defined in the right way

class Method(AWSObject):
    props = {
        "AuthorizationType": (str, False),  # Required
        "HttpMethod": (str, False),  # Required
        "ResourceId": (str, False),  # Required
        "RestApiId": (str, False)  # Required

    }

class Integration(AWSProperty):
    props = {
        # missing: "PassthroughBehavior": (str, False),
    }

@benbridts benbridts changed the title ApiGateway::Method validation doesn't match documentation ApiGateway resources do not match documentation Aug 18, 2016
@JaniszM
Copy link

JaniszM commented Aug 19, 2016

markpeek added a commit that referenced this issue Aug 19, 2016
@markpeek
Copy link
Member

@ikben @JaniszM I just did some quick edits for the missing prop and adjusting some of the required fields. Might be good for additional scrutiny on the names, types, and required fields.

@benbridts
Copy link
Contributor Author

benbridts commented Aug 19, 2016

Looks great!

I quickly went through the docs, and this is what caught my eye, I haven't tested anything:

class StageDescription(AWSProperty):
    props = {
        # ThrottlingRateLimit is defined in the docs as a Number, while others are defined as Integer, so this may need to be a check on float.
        "ThrottlingRateLimit": (positive_integer, False),
     }

class MethodSetting(AWSProperty):
    props = {
         # Same remark as above
        "ThrottlingRateLimit": (positive_integer, False)
    }

class Method(AWSObject):
    props = {
        # We could add extra validation to authorizationType
        # (f you specify the AuthorizerId property, specify CUSTOM for this property.)
        "AuthorizationType": (basestring, True),
    }

class Integration(AWSProperty):
    props = {
        "IntegrationHttpMethod": (basestring, False),  # Conditional
    }

class Model(AWSObject):
    props = {
        # Schema is a JSON object, so I think this should be (dict, False)
        "Schema": (basestring, False)
    }

class RestApi(AWSObject):
    props = {
        # Body is a JSON object, so I think this should be (dict, False)
        "Body": (basestring, False),
        "FailOnWarnings": (basestring, False),  # Should be (bool, False)
        "Name": (basestring, False), # Conditional
    }

amosshapira pushed a commit to amosshapira/troposphere that referenced this issue Oct 24, 2016
@aarcro
Copy link

aarcro commented Mar 14, 2017

Since this isn't closed yet, I'm going to add one additional point. From http://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-resource-apigateway-authorizer.html#cfn-apigateway-authorizer-authorizeruri

AuthorizerURI is only required for Lambda Authorizers, not for Type='COGNITO_USER_POOLS'

@bmurtagh
Copy link
Contributor

bmurtagh commented Jun 21, 2017

@aarcro how does bmurtagh@63a7133 and bmurtagh@eaf4a75#diff-4813259e14591fcd34ed4e0213482f00 look to address your AuthorizerURI issue?

The second commit just changes AuthorizerUri from being a required property to optional. The first commit has logic to ensure AuthorizerUri is set when Type is set to 'TOKEN'`.

@bmurtagh
Copy link
Contributor

I'm also taking a stab at the other issues @ikben has pointed out. That work will be done under the same branch followed by a PR. Hoping to start that tomorrow.

@aarcro
Copy link

aarcro commented Jun 21, 2017

@bmurtagh Looks good to me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants