-
Notifications
You must be signed in to change notification settings - Fork 7
SL-667: Add CICD config for Javabuilder demo env #385
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
Conversation
@molly-moen I still need to add something here, yeah? https://github.com/code-dot-org/javabuilder/blob/main/cicd/3-app/javabuilder/template.yml.erb#L1118 I specified the |
"LimitPerHour": "25", | ||
"LimitPerDay": "100", | ||
"SilenceAlerts": "false", | ||
"TeacherLimitPerHour": "5000" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is what you're discussing in other comments. For clarity, I'd suggest adding this to the other config files with whatever our default value is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But this would be safe to deploy as-is. You could also remove that config parameter at this time, and add it later as it's own PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In fact, that's what I'd recommend. Remove the TeacherLimitPerHour part from this PR, then we can merge this and make sure we've got a green pipeline into this new environment before adding new config parameters.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! To deploy this change, after merging, we just need to run the following.
cicd/2-cicd/deploy-cicd.sh
There might be a bit of pain to do with ruby versions or AWS credentials. Once you merge, switch back to main
on your local, pull this new code, then run cicd/2-cicd/deploy-cicd.sh
. Ping me on slack and I can follow along and help troubleshoot any issues.
Yep, updating Dashboard (and the frontend) would be a separate change - Dashboard reads the Javabuilder websocket and HTTP endpoints here and here (in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One question out of curiosity, but otherwise LGTM!
Configuration: | ||
StackName: | ||
!If [ | ||
TargetsMainBranch, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably a question for @cat5inthecradle - is it possible/should it be possible to target a branch other than main
for a production deploy?
Adds a new
javabuilder-demo
stack for Java Lab eval mode.Tickets:
Outstanding questions:
How do we modifyWill address this later per SL-667: Add CICD config for Javabuilder demo env #385 (comment).javabuilder/template.yml.erb
so that it doesn't use (or effectively ignores) theTeacherAssociatedRequests
table (here) and theTeacherLimitPerHour
(here)?