Updates for javabuilder-demo to go live#393
Conversation
molly-moen
requested review from
a team,
bencodeorg,
cat5inthecradle and
ebeastlake
| @@ -1 +1 @@ | |||
| 2.7.7 | |||
| 2.7.8 | |||
There was a problem hiding this comment.
This change was needed for the ci/cd pipeline to build correctly for a dev instance, so I think it will be needed for prod as well. We've done similar changes in the past to keep up the the AWS version of ruby.
| "BaseDomainNameHostedZonedID": "Z2LCOI49SCXUGU", | ||
| "ProvisionedConcurrentExecutions": "1", | ||
| "ReservedConcurrentExecutions": "5", | ||
| "ProvisionedConcurrentExecutions": "5", |
There was a problem hiding this comment.
I went with 5 here somewhat arbitrarily, lmk if we have another number we want to go with.
ebeastlake
left a comment
ebeastlake
left a comment
There was a problem hiding this comment.
This seems reasonable to me. I defer to @cat5inthecradle for the final word on the threshold numbers!
There was a problem hiding this comment.
You can't just name a PR "Final Updates" and think it's going to go through smoothly 🤣
Numbers look good from my perspective, and always easy to tweak. I have some feedback on the use of conditions instead of parameters for handling different environments. We should prefer parameters wherever possible. I think the desire to use conditionals here is partly because we have to work around the tech debt of manually-provisioned SNS topics.
EDIT: to be clear, my requested change is to get rid of IsDemoCondition and not add any environment-detecting logic to the template in this PR.
| Tracing: Active | ||
| Conditions: | ||
| IsDevCondition: !Equals [!Ref BaseDomainName, "dev-code.org"] | ||
| IsDemoCondition: !Equals [!Ref SubdomainName, "javabuilder-demo"] |
There was a problem hiding this comment.
I see you continued the existing pattern here, but we should try to keep this template environment-agnostic, and I would much rather refactor the design to allow us to eliminate the IsDevCondition rather than add a new IsEnvironmentCondition
You're using IsDemoCondition in two places, so I'll make comments directly there.
There was a problem hiding this comment.
PR to start eliminating IsDevCondition, definitely a non-school-year project ;)
#394
molly-moen
changed the title
😆 I renamed it to avoid cursing myself further |
* update limit and change alert description * try using version 2.7.5 * try using version 2.7.8 * add dev config * update alarm config * add whitespace * commas * try parens * hard-code limits * add period * temporarily change config for testing * change back to demo config * increase provisioned executions * remove dev config * switch to parameters * fix linting errors * add quotes * revert dev config change
3 participants
#infra-javabuilderrather than page the DOTD. I created a new SNS topic for this.I wanted to update the alarm to be based on a percentage of reserved concurrency, but I couldn't figure out a way to do that in a combination of CloudWatch and CloudFormation.