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

aws_sqs_queue - new resource #3674

Merged
merged 11 commits into from Dec 20, 2018

Conversation

Projects
None yet
3 participants
@amitsaha
Copy link
Contributor

amitsaha commented Dec 19, 2018

Support for AWS SQS queue support via a new aws_sqs_queue resource.

Closes #3675

@amitsaha amitsaha force-pushed the amitsaha:aws_sqs_queue branch from 47a4709 to ede17f6 Dec 19, 2018

@amitsaha amitsaha referenced this pull request Dec 19, 2018

Closed

Support AWS SQS Queue #3675

@clintoncwolfe clintoncwolfe changed the title RFC: Support for AWS SQS queue aws_sqs_queue - new resource Dec 19, 2018

@clintoncwolfe

This comment has been minimized.

Copy link
Contributor

clintoncwolfe commented Dec 19, 2018

Thank you for your contribution! However, before we can accept it, we ask that you sign your commits, to indicate that you are the author of the code and that it otherwise meets the terms of our Developer Certification of Origin policy.

After you have read the DCO, please sign your code, as follows:

git commit --amend -s
git push -f

Thank you!

@clintoncwolfe
Copy link
Contributor

clintoncwolfe left a comment

This looks like a great start!

To get this closer to being accepted, we'll need:

  • You'll need to run the linter to check for style issues. Run bundle exec rake lint to run the linter, and try bundle exec rubocop -a to auto-fix the issues.
  • User documentation. See docs/resources
  • Unit tests. These should focus on ensuring that the resource's properties and matchers work correctly, assuming a fake AWS response. See test/unit/resources/aws .
  • Integration tests. This tests the ability of the resource to speak to a real AWS account, and perform searches, etc. It does not need to exercise every feature, but should touch anything tricky. We use Terraform to setup and tear down things. See the integration test docs

If you need help on any of this, let us know!

@amitsaha

This comment has been minimized.

Copy link
Contributor

amitsaha commented Dec 19, 2018

Thanks @clintoncwolfe - will address those and update here once done.

Add more attributes
Signed-off-by: Amit Saha <amitsaha.in@gmail.com>

@amitsaha amitsaha force-pushed the amitsaha:aws_sqs_queue branch from bd33a27 to 3c2bb18 Dec 20, 2018

amitsaha added some commits Dec 20, 2018

Add unit test
Signed-off-by: Amit Saha <amitsaha.in@gmail.com>
Add docs
Signed-off-by: Amit Saha <amitsaha.in@gmail.com>
Update tests with FIFO queue checks
Signed-off-by: Amit Saha <amitsaha.in@gmail.com>
@amitsaha

This comment has been minimized.

Copy link
Contributor

amitsaha commented Dec 20, 2018

Progress on the review comments:

  • Lint issues
  • Unit test
  • Docs
  • Integration tests

Integration tests

Manually ran the specific integration test:

# Build
$ terraform apply -target="aws_sqs_queue.sqs_queue_1" -target="aws_sqs_queue.sqs_queue_2"

# Get output
$ terraform output > tf_outputs.attrs.yml

# Transform output to yaml file
sqs_queue_1_url : https://sqs.ap-southeast-2.amazonaws.com/519527725796/sqs_queue_1
sqs_queue_2_url : https://sqs.ap-southeast-2.amazonaws.com/519527725796/sqs_queue_2.fifo

Run tests:

> bundle exec inspec exec ..\verify\controls\aws_sqs_queue.rb  -t aws:// --attrs .\tf_outputs.attrs.yml

Profile: tests from ..\verify\controls\aws_sqs_queue.rb (tests from ...verify.controls.aws_sqs_queue.rb)
Version: (not specified)
Target:  aws://

  [PASS]  aws_sqs_queue lookup: aws_sqs_queue
     [PASS]  aws_sqs_queue should not exist
     [PASS]  aws_sqs_queue should exist
  [PASS]  aws_sqs_queue properties: aws_sqs_queue
     [PASS]  aws_sqs_queue delay_seconds should equal 0
     [PASS]  aws_sqs_queue is_fifo_queue should equal false
     [PASS]  aws_sqs_queue visibility_timeout should equal 300
     [PASS]  aws_sqs_queue maximum_message_size should equal 262144
     [PASS]  aws_sqs_queue message_retention_period should equal 345600
     [PASS]  aws_sqs_queue receive_message_wait_timeout_seconds should equal 2
  [PASS]  aws_sqs_queue fifo properties: aws_sqs_queue
     [PASS]  aws_sqs_queue is_fifo_queue should equal true
     [PASS]  aws_sqs_queue content_based_deduplication should equal true

Profile Summary: 3 successful controls, 0 control failure, 0 controls skipped
Test Summary: 10 successful, 0 failure, 0 skipped

@clintoncwolfe - should be good for review now. Thanks.

amitsaha added some commits Dec 20, 2018

Add integration test + add contentbaseddeduplication attribute
Signed-off-by: Amit Saha <amitsaha.in@gmail.com>
Fix for integration tests
Signed-off-by: Amit Saha <amitsaha.in@gmail.com>
Fix exception class for queue not found
Signed-off-by: Amit Saha <amitsaha.in@gmail.com>
Fix offenses
Signed-off-by: Amit Saha <amitsaha.in@gmail.com>
@clintoncwolfe
Copy link
Contributor

clintoncwolfe left a comment

Fantastic work! I have a few comments, but they are not enough to hold anything up. I'm running tests now, I'll approve if tests pass.

its('receive_message_wait_timeout_seconds') { should be 2 }
end

### is\_fifo\_queue

This comment has been minimized.

@clintoncwolfe

clintoncwolfe Dec 20, 2018

Contributor

Boolean properties should almost always be represented as matchers.

In other words, implement a method:

def a_fifo_queue?
  true # or whatever
end

This can then be called as:

    describe aws_sqs_queue('https://sqs.ap-southeast-2.amazonaws.com/1212121/MyQueue') do
        it { should_not be_a_fifo_queue }
    end

### content\_based\_deduplication

A boolean value indicate if content based dedcuplication is enabled or not

This comment has been minimized.

@clintoncwolfe

clintoncwolfe Dec 20, 2018

Contributor

As above, consider implementing as a matcher.

supports platform: 'aws'

include AwsSingularResourceMixin
attr_reader :arn, :is_fifo_queue, :visibility_timeout, :maximum_message_size, :message_retention_period, :delay_seconds, :receive_message_wait_timeout_seconds, :content_based_deduplication

This comment has been minimized.

@clintoncwolfe

clintoncwolfe Dec 20, 2018

Contributor

A minor thing, but we try to alphabetize long lists of attributes. Thanks!

Stale

@jerryaldrichiii
Copy link
Contributor

jerryaldrichiii left a comment

This looks good to me, willing to give it a 👍, but would like to see the matchers @clintoncwolfe mentioned.

@jerryaldrichiii

This comment has been minimized.

Copy link
Contributor

jerryaldrichiii commented Dec 20, 2018

Also, thank you @amitsaha I've seen you in a few places recently both here and in our Community Slack.

Your contributions are very much appreciated.

@clintoncwolfe
Copy link
Contributor

clintoncwolfe left a comment

As we're having unrelated trouble with our CI tests, I ran the unit, functional, and AWS integration tests manually - all pass. Thanks so much!

@clintoncwolfe

This comment has been minimized.

Copy link
Contributor

clintoncwolfe commented Dec 20, 2018

This PR contains a new resource, but we will not bump minor, as another PR on this release has already bumped 3.1 => 3.2 .

@clintoncwolfe clintoncwolfe merged commit bbc07f5 into inspec:master Dec 20, 2018

3 of 4 checks passed

continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details
DCO This commit has a DCO Signed-off-by
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
expeditor/config-validation Validated your Expeditor config file
Details
@amitsaha

This comment has been minimized.

Copy link
Contributor

amitsaha commented Dec 20, 2018

@amitsaha amitsaha deleted the amitsaha:aws_sqs_queue branch Jan 11, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment