Skip to content
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

roachprod: adding unit test and renaming ssh.go #64156

Merged
merged 1 commit into from
Aug 10, 2021

Conversation

alan-mas
Copy link
Contributor

@alan-mas alan-mas commented Apr 23, 2021

Third part of #47567 and fixes #65928 where we need to refactor ssh.go (as one type structure is in use, we cannot remove it completely).

So we are refactoring ssh.go and change its name to io.go and only keeping ProgressWriter struct and function.
Also we are adding some unit testing for this new go file.

Release note: None

@alan-mas alan-mas requested a review from jlinder April 23, 2021 20:32
@blathers-crl
Copy link

blathers-crl bot commented Apr 23, 2021

Thank you for contributing to CockroachDB. Please ensure you have followed the guidelines for creating a PR.

My owl senses detect your PR is good for review. Please keep an eye out for any test failures in CI.

🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is otan.

@blathers-crl blathers-crl bot added the O-community Originated from the community label Apr 23, 2021
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@alan-mas alan-mas added the do-not-merge bors won't merge a PR with this label. label Apr 23, 2021
@blathers-crl
Copy link

blathers-crl bot commented Apr 26, 2021

Thank you for updating your pull request.

My owl senses detect your PR is good for review. Please keep an eye out for any test failures in CI.

🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is otan.

@CLAassistant
Copy link

CLAassistant commented Apr 26, 2021

CLA assistant check
All committers have signed the CLA.

@alan-mas alan-mas changed the title roachprod: refactor ssh.go roachprod: adding unit test and renaming ssh.go May 14, 2021
@alan-mas alan-mas force-pushed the refactor-ssh-go branch 3 times, most recently from 2afa351 to 2fc40fe Compare June 2, 2021 14:39
@alan-mas alan-mas force-pushed the refactor-ssh-go branch 9 times, most recently from 1357454 to 9cbae10 Compare June 17, 2021 13:31
Copy link
Collaborator

@jlinder jlinder left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Reviewed 4 of 4 files at r1.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @alan-mas)

@alan-mas alan-mas requested a review from a team as a code owner July 13, 2021 21:56
@alan-mas alan-mas force-pushed the refactor-ssh-go branch 3 times, most recently from 9d7378a to eb0beda Compare July 20, 2021 19:23
@alan-mas alan-mas force-pushed the refactor-ssh-go branch 2 times, most recently from 06a3f38 to c0c0868 Compare July 28, 2021 16:33
@alan-mas alan-mas force-pushed the refactor-ssh-go branch 8 times, most recently from 3d10254 to 8c5b9e4 Compare August 5, 2021 19:33
Copy link
Collaborator

@jlinder jlinder left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 2 of 2 files at r4.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @alan-mas)


pkg/cmd/roachprod/ssh/io_test.go, line 54 at r4 (raw file):

Quoted 5 lines of code…
	defer func() {
		if err := os.Remove(output.Name()); err != nil {
			t.Fatal(err)
		}
	}()

Please put this right under the defer output.Close() near the beginning of this function.

Copy link
Collaborator

@jlinder jlinder left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @alan-mas and @jlinder)


pkg/cmd/roachprod/ssh/io_test.go, line 25 at r5 (raw file):

		t.Fatal(err)
	}
	fmt.Println(output.Name())

Why output the name of the file?

Copy link
Contributor Author

@alan-mas alan-mas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @jlinder)


pkg/cmd/roachprod/ssh/io_test.go, line 54 at r4 (raw file):

Previously, jlinder (James H. Linder) wrote…
	defer func() {
		if err := os.Remove(output.Name()); err != nil {
			t.Fatal(err)
		}
	}()

Please put this right under the defer output.Close() near the beginning of this function.

Done


pkg/cmd/roachprod/ssh/io_test.go, line 25 at r5 (raw file):

Previously, jlinder (James H. Linder) wrote…

Why output the name of the file?

Sorry for that I was printing the name to ensure the code was actually removing (so after running the test I can try to cat to file) and forgot to removing it. Just did it now.

Third part of cockroachdb#47567 where we need to refactor ssh.go (as one type structure is in use, we cannot remove it completely).

So we are refactoring ssh.go and change its name to io.go and only keeping ProgressWriter struct and function

Also we are adding a unit test go code to ensure it works as expected.

Release note: None
Copy link
Collaborator

@jlinder jlinder left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Reviewed 1 of 1 files at r7.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @alan-mas)

@jlinder
Copy link
Collaborator

jlinder commented Aug 10, 2021

bors r+

@craig
Copy link
Contributor

craig bot commented Aug 10, 2021

👎 Rejected by label

@jlinder jlinder removed the do-not-merge bors won't merge a PR with this label. label Aug 10, 2021
@jlinder
Copy link
Collaborator

jlinder commented Aug 10, 2021

bors r+

@craig
Copy link
Contributor

craig bot commented Aug 10, 2021

Build succeeded:

@craig craig bot merged commit f8956ee into cockroachdb:master Aug 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
O-community Originated from the community
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Roachprod: adding unit test and renaming ssh.go
4 participants