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

Add s3_bucket_notification module #58059

Open
wants to merge 1 commit into
base: devel
from

Conversation

@aljazkosir
Copy link

commented Jun 19, 2019

SUMMARY

PR to introduce lambda_bucket_event module.

ISSUE TYPE
  • New Module Pull Request
COMPONENT NAME
  • lib/ansible/modules/cloud/amazon/lambda_bucket_event.py
  • test/units/modules/cloud/amazon/test_lambda_bucket_event.py
ADDITIONAL INFORMATION

This module allows the management of AWS Lambda function bucket event mappings via the Ansible framework. Use module lambda to manage the lambda function itself, lambda_alias to manage function aliases and lambda_policy to modify lambda permissions.

Example:

---
# Example that creates a lambda event notification for a bucket
- hosts: localhost
  gather_facts: no
  tasks:
  - name: Process jpg image
    s3_bucket_notification:
      state: present
      event_name: on_jpg_add_or_remove
      bucket_name: test-bucket
      function_name: arn:aws:lambda:us-east-2:525810310200:function:test-lambda
      events: ["s3:ObjectCreated:*", "s3:ObjectRemoved:*"]
      prefix: images/
      suffix: .jpg
@ansibot

This comment has been minimized.

Copy link
Contributor

commented Jun 19, 2019

@aljazkosir, just so you are aware we have a dedicated Working Group for aws.
You can find other people interested in this in #ansible-aws on Freenode IRC
For more information about communities, meetings and agendas see https://github.com/ansible/community

click here for bot help

@aljazkosir

This comment has been minimized.

Copy link
Author

commented Jun 23, 2019

bot_status

@ansibot

This comment has been minimized.

Copy link
Contributor

commented Jun 23, 2019

Components

lib/ansible/modules/cloud/amazon/lambda_bucket_event.py
support: community
maintainers:

test/units/modules/cloud/amazon/test_lambda_bucket_event.py
support: core
maintainers:

Metadata

waiting_on: ansible
changes_requested_by: null
needs_info: False
needs_revision: False
needs_rebase: False
merge_commits: []
too many files or commits: False
mergeable_state: clean
shippable_status: success
maintainer_shipits (module maintainers): 0
community_shipits (namespace maintainers): 0
ansible_shipits (core team members): 0
shipit_actors (maintainer or core team member): []
shipit_actors_other: []
automerge: automerge shipit test failed

click here for bot help

@ansibot ansibot added the stale_ci label Jul 1, 2019

@aljazkosir aljazkosir closed this Jul 1, 2019

@aljazkosir aljazkosir reopened this Jul 1, 2019

@ansibot ansibot removed the stale_ci label Jul 1, 2019

@aljazkosir

This comment has been minimized.

Copy link
Author

commented Jul 2, 2019

ready_for_review

@willthames

This comment has been minimized.

Copy link
Contributor

commented Jul 3, 2019

This module seems the wrong approach - I'd prefer that this was just a parameter in the s3_bucket module

@aljazkosir aljazkosir force-pushed the xlab-si:add-lambda-bucket-event-module branch from ed3c2eb to 0fa373b Jul 11, 2019

@ansibot

This comment has been minimized.

Copy link
Contributor

commented Jul 11, 2019

The test ansible-test sanity --test future-import-boilerplate [explain] failed with 1 error:

lib/ansible/modules/cloud/amazon/lambda_bucket_event.py:0:0: missing: from __future__ import (absolute_import, division, print_function)

The test ansible-test sanity --test metaclass-boilerplate [explain] failed with 1 error:

lib/ansible/modules/cloud/amazon/lambda_bucket_event.py:0:0: missing: __metaclass__ = type

click here for bot help

@ansibot ansibot added needs_revision and removed core_review labels Jul 11, 2019

@aljazkosir aljazkosir force-pushed the xlab-si:add-lambda-bucket-event-module branch 2 times, most recently from b8147d9 to 845e36b Jul 11, 2019

@aljazkosir aljazkosir closed this Jul 11, 2019

@aljazkosir aljazkosir reopened this Jul 11, 2019

@aljazkosir aljazkosir force-pushed the xlab-si:add-lambda-bucket-event-module branch from 21c77d4 to 549f4db Jul 15, 2019

@aljazkosir

This comment has been minimized.

Copy link
Author

commented Jul 15, 2019

Thank you guys, I added a few more integration test and changed the name of the module.

@willthames @jillr should we add ourselves to BOTMETA.yml as module maintainers?

@aljazkosir aljazkosir force-pushed the xlab-si:add-lambda-bucket-event-module branch from 549f4db to b69d340 Jul 15, 2019

@aljazkosir aljazkosir changed the title Add lambda_bucket_event module Add s3_bucket_notification module Jul 15, 2019

@willthames

This comment has been minimized.

Copy link
Contributor

commented Jul 15, 2019

@aljazkosir you shouldn't need to as long as you're in the authors list - botmeta is only needed for overriding that.

lambda_function_arn:
description:
- The ARN of the lambda function.
required: true

This comment has been minimized.

Copy link
@willthames

willthames Jul 15, 2019

Contributor

Looking at this from a future proofing point of view, I'm not sure required should be true here (that also helps if you just want to delete a notification with only event_name and state)

lambda_function_arn=dict(default=None, aliases=['function_arn']),
bucket_name=dict(required=True),
events=dict(type='list', default=[], choices=event_types),
prefix=dict(required=False, default=''),

This comment has been minimized.

Copy link
@willthames

willthames Jul 15, 2019

Contributor

required=False is the default

events=dict(type='list', default=[], choices=event_types),
prefix=dict(required=False, default=''),
suffix=dict(required=False, default=''),
alias=dict(required=False, default=None),

This comment has been minimized.

Copy link
@willthames

willthames Jul 15, 2019

Contributor

default=None is the default

argument_spec=argument_spec,
supports_check_mode=True,
mutually_exclusive=[['alias', 'version']],
required_if=[['state', 'present', ['events', 'lambda_function_arn']]]

This comment has been minimized.

Copy link
@willthames

willthames Jul 15, 2019

Contributor

I'd like to remove lambda_function_arn in case anyone ever wants to implement other notification types

# ============================================================
# Test idempotency of CRUD

- name: test that configuration is the same as previous task

This comment has been minimized.

Copy link
@willthames

willthames Jul 15, 2019

Contributor

I'd like to see an idempotency check that order of events doesn't matter

@aljazkosir aljazkosir force-pushed the xlab-si:add-lambda-bucket-event-module branch from b69d340 to 90a9c29 Jul 16, 2019

@ansibot ansibot added needs_revision and removed core_review labels Jul 16, 2019

@aljazkosir aljazkosir force-pushed the xlab-si:add-lambda-bucket-event-module branch from 90a9c29 to 2ae70c5 Jul 16, 2019

@ansibot ansibot added core_review and removed needs_revision labels Jul 16, 2019

@aljazkosir

This comment has been minimized.

Copy link
Author

commented Jul 17, 2019

@willthames I updated the arguments and added test that checks that order of events doesn't matter

description:
- Name of the function alias. Mutually exclusive with C(version).
required: false
aliases: ['lambda_alias']

This comment has been minimized.

Copy link
@willthames

willthames Jul 17, 2019

Contributor

Can we just call this argument lambda_alias without the alias (just thinking about future proofing for other notification targets)

required: false
aliases: ['lambda_alias']
type: str
version:

This comment has been minimized.

Copy link
@willthames

willthames Jul 17, 2019

Contributor

Can we just call this argument lambda_version without the alias (just thinking about future proofing for other notification targets)

@@ -0,0 +1,2 @@
cloud/aws

This comment has been minimized.

Copy link
@willthames

willthames Jul 17, 2019

Contributor

Need to make the test suite name match the module name. I don't mind if that's adding aws_ to the module name or removing aws_ from the test suite name

This comment has been minimized.

Copy link
@aljazkosir

aljazkosir Jul 17, 2019

Author

Removing _aws from the test suite name seems better to me.

@aljazkosir aljazkosir force-pushed the xlab-si:add-lambda-bucket-event-module branch from 2ae70c5 to 39b5ed5 Jul 17, 2019

@willthames
Copy link
Contributor

left a comment

I've tested this locally, everything looks great. Good to merge from my point of view

@jillr
Copy link
Contributor

left a comment

Overall this looks good, but I'm concerned about the 41MB function.zip. We have to carry this around in the repo (and ansible is already fairly hefty), and I'm getting timeouts on the "register lambda" test some of the time. This will likely lead to these tests being unstable in CI. It would be better to use a simple hello world style script like test/integration/targets/aws_lambda/files/mini_lambda.py or test/integration/targets/lambda_policy/files/mini_http_lambda.py.

@ansibot ansibot added needs_revision and removed core_review labels Jul 17, 2019

@jillr

This comment has been minimized.

Copy link
Contributor

commented Jul 17, 2019

I'd also personally prefer it if the function were in-repo as a regular file, then zipped in the test, from a review and accounting perspective. It makes it much easier to track and ensure that nothing is sneaking into the repo via a binary.

- name: move lambda into place for archive module
copy:
src: "function.zip"
dest: "{{output_dir}}/function.zip"

This comment has been minimized.

Copy link
@mattclay

mattclay Jul 17, 2019

Member

We should not include a file of this size in the repo. Use a very tiny lambda function and create the zip file during the test. If that's not possible, then we can put the zip file in our S3 bucket for CI -- but lets avoid that if possible.

This comment has been minimized.

Copy link
@mattclay

mattclay Jul 17, 2019

Member

Ideally the lambda would be a singe python file with no requirements outside the standard library, then no zip would be needed. Keeping it python vs another language makes it easier to review since most contributors will be familiar with python.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.