Improved configuration of the tool

assertible / lambda-cloudwatch-slack

Send AWS CloudWatch notifications to a Slack channel using Lambda

https://assertible.com/blog/npm-package-lambda-cloudwatch-slack

MIT License

482 stars 249 forks source link

Improved configuration of the tool #30

Closed proAlexandr closed 5 years ago

proAlexandr commented 5 years ago

No more multiple .env files.
Removed unnecessary env variables, such as Channel name, slack username etc, because it should be configured through the Slack UI during webhook configuration.
Now deployment work without installed AWS CLI. You fill access key and secret at .env file
Cloudwatch notifications don’t require specific SNS topic name to have a proper parsing.
Makefile changed to npm scripts to keep consistent and run everything through npm.

CodyReichert commented 5 years ago

Oh this looks very exciting and very much much needed :D Thanks for the PR

Give me a couple of hours and I'll pull it and test everything out and leave any comments for you. :+1:

lnikell commented 5 years ago

@CodyReichert Also we wrote the article with full installation instructions, so next week it will be published and we will submit another PR with the link with a full guide.

creichert commented 5 years ago

This is fantastic work. And I will definitely merge it. I have a few observations:

I use Makefiles extensively on every project. But mainly to automate my own commands. I think it's good to remove it as it can confuse many people used to simple nodejs infrastructure. :+1:

@CodyReichert Also we wrote the article with full installation instructions, so next week it will be published and we will submit another PR with the link with a full guide.

Do you want us to wait to pull this in until you release your blog? Either way, definitely open a new PR when you do.
Can you fix the simple review comments? If you disagree with any, that's okay they are not blockers.

Additionally, it would be great if you and your team could help us keep improving the per-service notifications to be even more accurate.

All-in-all, I'm very happy about this PR. Thank you!

proAlexandr commented 5 years ago

@creichert @CodyReichert

~Do we need to have package-lock.json in git? Or I can add it to .gitignore?~
I prefer if PR be merged with squash to not store unnecessarily changes like
```
in git. Do you agree?
```

creichert commented 5 years ago

@proAlexandr I can rebase them, but in the future you could do an interactive rebase locally:

git rebase -i HEAD~6 # rebase number of commits on that base branch

Then, squash all commits into the last one and git push -f origin branch. No worries, though, just saying that is possible.

creichert commented 5 years ago

@proAlexandr fantastic contribution, I can add you as a collaborator on this repo so you can more quickly make changes and approve pull requests, if you want? We'd still like to have a PR for all changes.

proAlexandr commented 5 years ago

I can add you as a collaborator on this repo so you can more quickly make changes and approve pull requests, if you want?

@creichert Thank you, but for now, I think it is not necessary.