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

STREAM-437 - Create image with serverless-plugin-aws-alerts installed #29

Closed
wants to merge 1 commit into from

Conversation

taro-bh
Copy link

@taro-bh taro-bh commented Apr 6, 2022

This PR creates a serverless image with the new dependency (https://www.serverless.com/plugins/serverless-plugin-aws-alerts) installed. The attempt is made to avoid affecting the existing images at all, in order to avoid potential blow up in production.

The need for the new dependency arose due to https://github.com/sleepio/flows-cli/pull/33 in the context of https://bighealth.atlassian.net/browse/STREAM-437, where we are adding baseline configuration for CloudWatch metrics alerts.

I'd like to loop in the DevOps team to see if this approach is reasonable and supportable.

@nmguse-bighealth
Copy link

I do not see the need to create a separate image for this, unless there was something I overlooked which would knowingly affect existing builds. This looks purely additive as a plugin.

I'm guessing you'll at some point need to have a way to chose the proper sns topics, to have them be passed into the builds. Is there a plan for how to manage that yet (we can discuss via slack instead on this topic)

@ipanousis
Copy link

@nmguse-bighealth So you're happy with us overwriting the currently used image with this new plugin installed by default? We were just being extra cautious to not affect the other services but we're open to your guidance on this.

To your question on alarm configuration management, we're working on that here in the flows deployment tooling, we're adding some alarms by default and in subsequent work we'll be permitting configuration for alarms on custom metrics

@nmguse-bighealth
Copy link

Yes, I do not see a reason we should not add this to the base image, it seems potentially useful in general, and I don't see a significant risk to existing usage

@taro-bh
Copy link
Author

taro-bh commented Apr 6, 2022

I have created a PR that adds the new dependency, instead of creating a newly tagged image: #30

That one surely is simpler, and if we can ensure that it will cause no issue then it may well be preferred over this PR. I wonder how we test before merge.

@nmguse-bighealth
Copy link

Merge it sometime before noon PT tomorrow and it'll be used in the release cut off deployment to stage

@taro-bh
Copy link
Author

taro-bh commented Apr 11, 2022

Closing, as #30 takes it over.

@taro-bh taro-bh closed this Apr 11, 2022
@taro-bh taro-bh deleted the stream-437/stopgap-image branch April 12, 2022 00:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants