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

feat: add lint and unit test workflow checks for pull requests #152

Merged

Conversation

bryantbiggs
Copy link
Member

@bryantbiggs bryantbiggs commented Dec 9, 2021

Description

  • add lint, type check, and unit test checks to CI workflow
    • test snapshots are added to aid in visually inspecting what has changed since we are converting some JSON payload into another JSON type payload that will be sent to Slack
  • replace Python map of mixed message payloads with discrete JSON payloads separated by file and by type. JSON event payloads that are in the SNS message format are stored under functions/messages/, and JSON raw event message payloads are stored under functions/events/*. SNS messages are what are currently trigger the lambda handler, but by having the raw events as well we can make developing and testing custom messages for new event types easier; in the end they are just stringified JSON payloads store din the Message attribute of the SNS message that triggers the function
  • the slack-notify-simple example has been updated to produce an environment variables file to aid in local integrations testing
  • an integration testing file integration_test.py has been added to test the resources live (using the SNS topic and Lambda handler created by the slack-notify-simple example project)
  • documentation has been updated to explain and demonstrate how to work with the Python codebase and tests locally

Motivation and Context

  • Improve testing facilities to aid in pull request reviews; next step is to start adding more event types supported

Breaking Changes

  • No

How Has This Been Tested?

  • I have tested and validated these changes using one or more of the provided examples/* projects
    • Unit tests => see GitHub action workflow results or run pipenv run test
    • Integration test => deployed slack-notify-simple project using my provided webhook URL and then ran pipenv run python integration_test.py

@bryantbiggs
Copy link
Member Author

and it fails, fantastic 🎉! time to fix


events = (
(
{
Copy link
Member Author

Choose a reason for hiding this comment

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

this threw me for a loop. some events are shown in the SNS event format thats delivered to the lambda when invoked, and others are in the raw message format (that would in theory be passed to something like aws sns publish --topic-arn "xxx" --message "<message>").

Its this 2nd format, the raw format, that had me scratching my head as to why this https://github.com/terraform-aws-modules/terraform-aws-notify-slack/blob/master/functions/notify_slack.py#L153 wasn't failing because they don't have an enclosing SNS event format with Records: []. But the current tests don't test through the actual lambda handler, they short-circuit through this function https://github.com/terraform-aws-modules/terraform-aws-notify-slack/blob/master/functions/notify_slack.py#L101

functions/notify_slack.py Show resolved Hide resolved
.gitignore Show resolved Hide resolved
@bryantbiggs bryantbiggs marked this pull request as ready for review December 10, 2021 20:15
@antonbabenko antonbabenko merged commit d2675ec into terraform-aws-modules:master Dec 10, 2021
antonbabenko pushed a commit that referenced this pull request Dec 10, 2021
# [4.22.0](v4.21.0...v4.22.0) (2021-12-10)

### Features

* add lint and unit test workflow checks for pull requests ([#152](#152)) ([d2675ec](d2675ec))
@antonbabenko
Copy link
Member

This PR is included in version 4.22.0 🎉

@antonbabenko
Copy link
Member

Very nice! Glad to see some more improvements in this rather popular module. Thanks @bryantbiggs !

@bryantbiggs bryantbiggs deleted the feat/ci-unit-tests branch December 10, 2021 21:06
@github-actions
Copy link

github-actions bot commented Nov 8, 2022

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 8, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants