-
Notifications
You must be signed in to change notification settings - Fork 0
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
Launch smoke tests via ECS #35
Conversation
cd/manager/jobs/smoke.go
Outdated
const SmokeTestFailureTime = 1 * time.Hour | ||
const SmokeTaskIdParam = "id" | ||
|
||
const ECSCluster = "ceramic-qa-tests" |
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.
Might be worth moving these to models.go
too? Just a thought.
const TestsCluster = "ceramic-qa-tests"
const TestsNetworkConfigurationParameter = "network_configuration"
const (
TestType_Smoke string = "smoke"
TestType_E2e string = "e2e"
)
and then the code in smoke.go
becomes:
if id, err := s.d.LaunchTask(
manager.TestsCluster,
manager.TestsCluster+"-"+manager.TestType_Smoke+"-"+s.env,
manager.TestsCluster+"-"+manager.TestType_Smoke,
"/"+manager.TestsCluster+"-"+manager.TestType_Smoke+"/"+manager.TestsNetworkConfigurationParameter,
nil); err != nil {
Something like that. Just a thought. Could also use fmt.sprintf
to make the string formats a little more explicit and readable. e.g.
fmt.Sprintf("%s-%s-%s", manager.TestsCluster, manager.TestType_Smoke, s.env) // for family
fmt.Sprintf("/%s-%s/%s", manager.TestsCluster, manager.TestType_Smoke, manager.TestsNetworkConfigurationParameter) // for n/w config
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 the benefit of having it typed out when you read the code is pretty large
As in, when I see fmt.Sprintf("%s-%s--%s", manager.TestsCluster, manager.TestType_Smoke, s.env)
it will take me a minute, and flipping back and forth between files, to figure out what the resulting string actually is
vs if I can see const ECSFamilyPrefix = "ceramic-qa-tests-smoke--"
and ECSFamilyPrefix+s.env,
on the same page. It's much more obvious what the vaule 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.
pulling out const TestsCluster = "ceramic-qa-tests"
makes sense for next pr when we migrate the e2e tests
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.
Ok, sg. Should take ECS
out of the literal names though to keep the abstractions cleaner. The jobs don't care that ECS is being used under the covers.
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 apart from minor editorial comment 🚀
Summary
Launching smoke tests with ECS instead of lambda because of https://github.com/3box/ceramic-smoke-tests/pull/30
Motivation
When we upgraded our Ceramic packages to ESM, smoke tests broke in lambda. Dockerizing them and running on ECS makes it easier to handle upgrades. (More details in PR referenced above).
Changes
TaskIdParam
const to be prefixed with the job name