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

roachtest: fix roachtest --cluster #31401

Merged
merged 1 commit into from Oct 15, 2018

Conversation

Projects
None yet
4 participants
@andreimatei
Member

andreimatei commented Oct 15, 2018

A recent commit delayed the setting of c.t, but some functions used by
attachToExistingCluster() were not prepated to work without c.t set.
This test introduces c.StartE/WipeE flavors that can be run on a cluster
that hasn't a test set yet.

Release note: None

@cockroach-teamcity

This comment has been minimized.

Show comment
Hide comment
@cockroach-teamcity

cockroach-teamcity Oct 15, 2018

Member

This change is Reviewable

Member

cockroach-teamcity commented Oct 15, 2018

This change is Reviewable

@andreimatei

This comment has been minimized.

Show comment
Hide comment
@andreimatei

andreimatei Oct 15, 2018

Member

not ready yet, sorry

Member

andreimatei commented Oct 15, 2018

not ready yet, sorry

@BramGruneir

This comment has been minimized.

Show comment
Hide comment
@BramGruneir

BramGruneir Oct 15, 2018

Member

Perhaps we should revert the change that broke it and then you can take your time fixing this.

Member

BramGruneir commented Oct 15, 2018

Perhaps we should revert the change that broke it and then you can take your time fixing this.

@andreimatei

This comment has been minimized.

Show comment
Hide comment
@andreimatei

andreimatei Oct 15, 2018

Member

ready for review now

Member

andreimatei commented Oct 15, 2018

ready for review now

@BramGruneir

:lgtm: With one comment.

Reviewed 1 of 1 files at r1.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (and 1 stale)


pkg/cmd/roachtest/cluster.go, line 747 at r1 (raw file):

	if !opt.skipStop {
		c.status("stopping cluster")
		c.All()

Why do you need to call all() here?

@benesch

:lgtm: as well

Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (and 1 stale)


pkg/cmd/roachtest/cluster.go, line 747 at r1 (raw file):

Previously, BramGruneir (Bram Gruneir) wrote…

Why do you need to call all() here?

Yeah, this looks spurious.

@andreimatei

TFTRs

bors r+

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (and 2 stale)


pkg/cmd/roachtest/cluster.go, line 747 at r1 (raw file):

Previously, benesch (Nikhil Benesch) wrote…

Yeah, this looks spurious.

debug leftover, sorry

craig bot pushed a commit that referenced this pull request Oct 15, 2018

Merge #31401
31401: roachtest: fix roachtest --cluster r=andreimatei a=andreimatei

A recent commit delayed the setting of c.t, but some functions used by
attachToExistingCluster() were not prepated to work without c.t set.
This test introduces c.StartE/WipeE flavors that can be run on a cluster
that hasn't a test set yet.

Release note: None

Co-authored-by: Andrei Matei <andrei@cockroachlabs.com>
@craig

This comment has been minimized.

Show comment
Hide comment
@craig

craig bot commented Oct 15, 2018

Build failed

roachtest: fix roachtest --cluster
A recent commit delayed the setting of c.t, but some functions used by
attachToExistingCluster() were not prepated to work without c.t set.
This test introduces c.StartE/WipeE flavors that can be run on a cluster
that hasn't a test set yet.

Release note: None
@andreimatei

This comment has been minimized.

Show comment
Hide comment
@andreimatei

andreimatei Oct 15, 2018

Member

bors r+

Member

andreimatei commented Oct 15, 2018

bors r+

craig bot pushed a commit that referenced this pull request Oct 15, 2018

Merge #31401
31401: roachtest: fix roachtest --cluster r=andreimatei a=andreimatei

A recent commit delayed the setting of c.t, but some functions used by
attachToExistingCluster() were not prepated to work without c.t set.
This test introduces c.StartE/WipeE flavors that can be run on a cluster
that hasn't a test set yet.

Release note: None

Co-authored-by: Andrei Matei <andrei@cockroachlabs.com>
@craig

This comment has been minimized.

Show comment
Hide comment
@craig

craig bot commented Oct 15, 2018

Build failed

@andreimatei

This comment has been minimized.

Show comment
Hide comment
@andreimatei

andreimatei Oct 15, 2018

Member

flake pinged to Nikhil.

One more spin:
bors r+

Member

andreimatei commented Oct 15, 2018

flake pinged to Nikhil.

One more spin:
bors r+

craig bot pushed a commit that referenced this pull request Oct 15, 2018

Merge #31401
31401: roachtest: fix roachtest --cluster r=andreimatei a=andreimatei

A recent commit delayed the setting of c.t, but some functions used by
attachToExistingCluster() were not prepated to work without c.t set.
This test introduces c.StartE/WipeE flavors that can be run on a cluster
that hasn't a test set yet.

Release note: None

Co-authored-by: Andrei Matei <andrei@cockroachlabs.com>
@craig

This comment has been minimized.

Show comment
Hide comment
@craig

craig bot commented Oct 15, 2018

Build succeeded

@craig craig bot merged commit 83e9084 into cockroachdb:master Oct 15, 2018

3 checks passed

GitHub CI (Cockroach) TeamCity build finished
Details
bors Build succeeded
Details
license/cla Contributor License Agreement is signed.
Details

@andreimatei andreimatei deleted the andreimatei:roachtest-attach-fix branch Oct 15, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment