-
Notifications
You must be signed in to change notification settings - Fork 98
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
Running all the tests on two clusters and only full-rr on 3 clusters #769
Conversation
3b4c31e
to
f123315
Compare
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.
.github/workflows/terratest-more-clusters.yaml
will be executed simultaneousy with terratest.yaml workflow ?
} | ||
|
||
// helper function for de-duplicating elements in slice (in O(n)) | ||
func dedupe(s []string) (uniq []string) { |
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.
please rename the function to something more meaningful (e.g. in SQL wa have distinct keyword for that)
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.
changed to distinct
if n < 2 || n > 8 { | ||
t.Logf("Use value of n that represents the number of clusters from interval [2,8]") | ||
t.FailNow() | ||
} else { | ||
t.Logf(fmt.Sprintf("Running TestFullRoundRobin for %d clusters", n)) | ||
} |
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.
avoid if else statements if you can, just invert condition and fail fast (e.g: crossplane doc).
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.
right, the else branch is totally useless here :D (done)
f123315
to
0a23ee2
Compare
y, that's how it's done in this PR. But I am open to also run it either on demand by dispatch action or nightly/"morningly" (cron), but as it was mentioned on the call, this way nobody will pay attention to the result and it may be hard to track the change that introduced some issue. Btw. it runs only |
Signed-off-by: Jirka Kremser <jiri.kremser@gmail.com>
0a23ee2
to
2c448c4
Compare
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, thanks for the fixes!
Making
TestFullRoundRobin
dynamic in a way that it can run the assertions properly for any number of clusters (any ~ [2,8])In makefile we should pass the build tag
all
xorrr_multicluster
based on the number of clusters that currently runs.all
will run the all the tests (same as before).rr_multicluster
will run only the TestFullRoundRobin that depends onCLUSTERS_NUMBER
env variable (which defaults to2
).This way, we can also have a on dispatch GH action that can run the tests on-demand w/ the number of clusters as an input parameter of the action (not done in this PR).
Some data:
I've tried 4 attempts for these pipelines:
All 12 passed so I am assuming it's better than current master where it's pretty fragile.
Signed-off-by: Jirka Kremser jiri.kremser@gmail.com