Join GitHub today
GitHub is home to over 28 million developers working together to host and review code, manage projects, and build software together.
Sign uproachtest: cluster ctors no longer take a test #31267
Conversation
andreimatei
assigned
petermattis
Oct 11, 2018
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
|
This change is |
petermattis
approved these changes
Oct 12, 2018
Reviewable status:
complete! 0 of 0 LGTMs obtained
pkg/cmd/roachtest/test_test.go, line 272 at r1 (raw file):
{[]string{"hello+"}, "must match regex"}, {[]string{strings.Repeat("y", 46)}, ""}, {[]string{strings.Repeat("y", 100)}, "must match regex"},
Why did this change? The cluster name restriction is imposed on us by GCE.
andreimatei
reviewed
Oct 12, 2018
Reviewable status:
pkg/cmd/roachtest/test_test.go, line 272 at r1 (raw file):
Previously, petermattis (Peter Mattis) wrote…
Why did this change? The cluster name restriction is imposed on us by GCE.
it changed because, as it was written, it was no longer failing because the cluster names that were generated based on the test input became a bit shorter (because the "cluster id" component of the name used to be set to a timestamp or something transparently inside the name creation functions, and now the caller is supposed to put it in the name (it it wants to).
I didn't compute the new magic 47 that breaks the camel's back and makes the name too long; instead I just verify that a really long name is rejected. I think it's sufficient, but I'll change if you want.
andreimatei
reviewed
Oct 12, 2018
I've had to lift the prepending of the username to the cluster name from makeClusterName(). It was updating a global that the racetest caught. I don't know why it only caught it now; I think it was always racy. Globals are evil.
PTAL.
Reviewable status:
petermattis
approved these changes
Oct 15, 2018
Reviewable status:
bot
pushed a commit
that referenced
this pull request
Oct 15, 2018
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
craig
bot
commented
Oct 15, 2018
Build succeeded |
andreimatei commentedOct 11, 2018
The divorcing of clusters from tests is now unavoidable, and this patch
is another step in that direction.
This patch changes the cluster ctors to not take a test at all. Instead,
c.setTest() needs to be called later to set c.t. The idea is that it can
be called multiple times on the same cluster (and also that eventually
it will not be needed at all).
Release note: None