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

Allow an empty string as an SNS ARN and ignore it #76

Conversation

@chrisgilmerproj
Copy link
Contributor

commented Sep 6, 2019

This change allows a user to pass in an empty string as an environment variable for the SNS ARN which will have the same effect as not setting that environment variable.

Background: I am setting up this lambda using a terraform module. In terraform I have to list all the environment variables that I might wish to change and some of them end up being empty strings. The sns_client.publish() function expects a real SNS ARN and not an empty string or it won't validate. The fix here is to check if the environment variable was set and if so an empty string will be ignored similar to if the environment variable was never set.

@CLAassistant

This comment has been minimized.

Copy link

commented Sep 6, 2019

CLA assistant check
All committers have signed the CLA.

@chrisgilmerproj

This comment has been minimized.

Copy link
Contributor Author

commented Sep 6, 2019

I should mention that I have tried this in AWS with lambda and it did work as expected.

@chrisgilmerproj chrisgilmerproj referenced this pull request Sep 9, 2019
@chrisgilmerproj

This comment has been minimized.

Copy link
Contributor Author

commented Sep 10, 2019

@denniswebb @edhgoose - I just realized that I spammed you folks with 5 different PRs but it appears that the repo might be closed? I don't see a lot of PRs being accepted or worked on in the last year. I'm actively working on this now for a client so I could just fork this permanently but I am hoping to give back here since this is a really nice solution you all have shared. Any thoughts?

@chrisgilmerproj

This comment has been minimized.

Copy link
Contributor Author

commented Sep 10, 2019

cc @jaygorrell who may have been more recently in this repo :) Sorry for the spam folks.

@jaygorrell

This comment has been minimized.

Copy link
Contributor

commented Sep 10, 2019

Hey @chrisgilmerproj, you're right the project has gotten a little stale. We would love for the contributions to still happen here so we can build this thing up but the biggest issue right now is the lack of tests and capacity to handle PRs without them -- if we get those in, reviews would be much easier.

I'll see if we can get some eyes on your changes and talk to some folks here to see what we can do about maintaining the project.

@jaygorrell
Copy link
Contributor

left a comment

This one is easy enough though :)

@jaygorrell jaygorrell merged commit 048613e into upsidetravel:master Sep 10, 2019

2 checks passed

ci/circleci Your tests passed on CircleCI!
Details
license/cla Contributor License Agreement is signed.
Details
@chrisgilmerproj

This comment has been minimized.

Copy link
Contributor Author

commented Sep 11, 2019

@jaygorrell - thank you so much for reviewing these PRs! I'll make you a deal and any future PRs will come with tests so that you can more quickly review them. I'll also confirm that they work on my system in the PR before submitting so that you know someone's practically using the change.

@chrisgilmerproj chrisgilmerproj deleted the chrisgilmerproj:cg_allow_empty_string_for_sns_arn branch Sep 11, 2019

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