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

New Skeletal Resource aws_sns_subscription #2697

Merged
merged 4 commits into from
Mar 22, 2018

Conversation

dromazmj
Copy link
Contributor

This PR adds functionality for a new resource that allows testing against a SNS Subscription.

Signed-off-by: Matthew Dromazos dromazmj@dukes.jmu.edu

Signed-off-by: Matthew Dromazos <dromazmj@dukes.jmu.edu>
@dromazmj dromazmj requested a review from a team as a code owner February 19, 2018 18:09
@jquick jquick changed the base branch from release-2.0 to master February 19, 2018 18:55
@clintoncwolfe clintoncwolfe added Platform: AWS Amazon Web Services-related issues enhancement labels Feb 20, 2018
Signed-off-by: Matthew Dromazos <dromazmj@dukes.jmu.edu>
Copy link
Contributor

@clintoncwolfe clintoncwolfe left a comment

Choose a reason for hiding this comment

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

Great work @dromazmj ! A few docs changes, and one matcher I'd like to see aliased for better fluency, but no substantial code changes. Unit and integration tests pass. Great contribution!


Provides the Subscriptions protocol used. For example http, https, email, email-json, sqs, etc. For more information about protocols please visit https://docs.aws.amazon.com/sns/latest/api/API_Subscribe.html

# Inspect the endpoint
Copy link
Contributor

Choose a reason for hiding this comment

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

Protocol, not endpoint


# Inspect the endpoint
describe aws_sns_subscription(subscription_arn: 'arn:aws:sns:us-east-1::test-topic-01:b214aff5-a2c7-438f-a753-8494493f2ff6' ) do
its('endpoint') { should cmp 'arn:aws:sns:us-east-1::test-topic-02' }
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think SNS Topics are valid as endpoints - see the AWS docs. You might give a few examples that vary based on protocol:

  # If protocol is 'sms', this should be a phone number:
  its('endpoint') { should cmp '+16105551234' }
  # If protocol is 'email', endpoint should be an email address
  its('endpoint') { should cmp 'nobody@example.com' }

it { should_not exist }
end

### be_confirmation_authenticated
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it would read better as require_authenticated_confirmation. You'd have to use `RSpec::Matcher.alias_matcher to get the name right (see an example).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this matcher is saying whether or not the confirmation was authenticated not exactly required. Maybe be_authenticated or has_been_authenticated

Copy link
Contributor Author

Choose a reason for hiding this comment

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

def fetch_from_api
backend = BackendFactory.create(inspec_runner)

@aws_response = backend.get_subscription_attributes(subscription_arn: @subscription_arn).attributes
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't make aws_response an instance variable - it's a local utility variable, not a property of the AwsSnsSubscription object.

it { should exist }
end

# describe aws_sns_subscription(subscription_arn: 'arn:aws:sns:us-east-1:721741954427:NonExistentSubscrtiption:bf007420-6-45-96-9c2bf144') do
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this commented out?

data "aws_caller_identity" "current" {}

output "account_id" {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is already available - check aws.tf

}),
}),
})
# require 'pry'
Copy link
Contributor

Choose a reason for hiding this comment

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

:) Please remove

* Clarifies documentation
* Wraps calls to aws api in catch_aws_errors metho
* Fixes integration tests

Signed-off-by: Matthew Dromazos <dromazmj@dukes.jmu.edu>
Copy link
Contributor

@jquick jquick left a comment

Choose a reason for hiding this comment

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

Thanks @dromazmj ! Really nice addition.

@jquick jquick dismissed clintoncwolfe’s stale review March 22, 2018 14:26

Reviewer Unavailable

Copy link
Contributor

@clintoncwolfe clintoncwolfe left a comment

Choose a reason for hiding this comment

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

Looks great - thanks @dromazmj !

# If protocal is 'http', endpoint should be a URL beginning with 'https://'
its('endpoint') { should cmp 'https://www.exampleurl.com' }
# If the protocol is 'lambda', its endpoint should be the ARN of a AWS Lambda function
its('endpoint') { should cmp 'rn:aws:lambda:us-east-1:account-id:function:myfunction' }
Copy link
Contributor

Choose a reason for hiding this comment

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

💯

@jquick jquick merged commit 9077a7b into inspec:master Mar 22, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Platform: AWS Amazon Web Services-related issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants