Updates for javabuilder-demo to go live #393
Conversation
molly-moen
requested review from
cat5inthecradle,
bencodeorg,
ebeastlake and
a team
| @@ -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.
| @@ -3,8 +3,8 @@ | |||
| "BaseDomainName": "code.org", | |||
| "SubdomainName": "javabuilder-demo", | |||
| "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.
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.
| @@ -63,6 +63,7 @@ Globals: | |||
| 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 |
#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.