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.

Sign up for GitHub

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

Jump to bottom

Fix nil pointer dereference in test framework #37583

Merged
merged 1 commit into from
Nov 30, 2016
Merged

Fix nil pointer dereference in test framework #37583

merged 1 commit into from
Nov 30, 2016

Conversation

mtaufen
Copy link
Contributor

@mtaufen mtaufen commented Nov 28, 2016 •
edited by k8s-oncall
Loading

Checking the result.Code prior to err in the if statement causes a panic if result is nil. It turns out the formatting of the error is already in IssueSSHCommandWithResult, so removing redundant code is enough to fix the issue. Logging the SSH result was also redundant, so I removed that as well.

This change is Reviewable

@mtaufen mtaufen added area/test kind/bug Categorizes issue or PR as related to a bug. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. release-note-none Denotes a PR that doesn't merit a release note. labels Nov 28, 2016
@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Nov 28, 2016
Random-Liu
Random-Liu reviewed Nov 28, 2016
View reviewed changes
test/e2e/framework/util.go
if result.Code != 0 || err != nil {
return fmt.Errorf("failed running %q: %v (exit code %d)",
cmd, err, result.Code)
if err != nil {
Copy link
Member

Choose a reason for hiding this comment

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

If so, result will never be used. Are you sure golang will not throw out an error?

Copy link
Member

Choose a reason for hiding this comment

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

The code in IssueSSHCommandWithResult should also be changed.
Check err first then check result.Code.

Copy link
Member

Choose a reason for hiding this comment

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

Discussed offline, result is not a pointer in IssueSSHCommandWithResult.

@k8s-github-robot k8s-github-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Nov 28, 2016
@Random-Liu
Copy link
Member

Is there a corresponding issue for this?

@mtaufen
Fix nil pointer dereference in test framework
f789ecd 
Checking the result.Code prior to err in the if statement causes a panic
if result is nil. It turns out the formatting of the error is already in
IssueSSHCommandWithResult, so removing redundant code is enough to fix
the issue. Logging the SSH result was also redundant, so I removed that
as well.
@k8s-ci-robot
Copy link
Contributor

Jenkins GCE Node e2e failed for commit f7ea225. Full PR test history.

The magic incantation to run this job again is @k8s-bot node e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

@Random-Liu Random-Liu added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 28, 2016
@Random-Liu
Copy link
Member

LGTM

@k8s-ci-robot
Copy link
Contributor

Jenkins CRI GCE Node e2e failed for commit f7ea225. Full PR test history.

The magic incantation to run this job again is @k8s-bot cri node e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

@k8s-ci-robot
Copy link
Contributor

Jenkins Kubemark GCE e2e failed for commit f7ea225. Full PR test history.

The magic incantation to run this job again is @k8s-bot kubemark e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

@k8s-ci-robot
Copy link
Contributor

Jenkins GCE e2e failed for commit f7ea225. Full PR test history.

The magic incantation to run this job again is @k8s-bot cvm gce e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

@k8s-ci-robot
Copy link
Contributor

Jenkins GCE etcd3 e2e failed for commit f7ea225. Full PR test history.

The magic incantation to run this job again is @k8s-bot gce etcd3 e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

@k8s-ci-robot
Copy link
Contributor

Jenkins kops AWS e2e failed for commit f7ea225. Full PR test history.

The magic incantation to run this job again is @k8s-bot kops aws e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

@k8s-ci-robot
Copy link
Contributor

Jenkins GKE smoke e2e failed for commit f789ecd. Full PR test history.

The magic incantation to run this job again is @k8s-bot cvm gke e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

@k8s-ci-robot
Copy link
Contributor

Jenkins GCI GKE smoke e2e failed for commit f789ecd. Full PR test history.

The magic incantation to run this job again is @k8s-bot gci gke e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

@mtaufen mtaufen mentioned this pull request Nov 28, 2016
Closed
@mtaufen
Copy link
Contributor Author

mtaufen commented Nov 29, 2016

@k8s-bot gke e2e test this

@mtaufen
Copy link
Contributor Author

mtaufen commented Nov 29, 2016

@k8s-bot unit test this

@mtaufen
Copy link
Contributor Author

mtaufen commented Nov 30, 2016

@lavalamp Any idea why the unit/integration job is stuck?

@mtaufen
Copy link
Contributor Author

mtaufen commented Nov 30, 2016

@k8s-bot unit test this

@mtaufen
Copy link
Contributor Author

mtaufen commented Nov 30, 2016

/cc @fejta

@k8s-github-robot
Copy link

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

@k8s-github-robot
Copy link

Automatic merge from submit-queue

@k8s-github-robot k8s-github-robot merged commit 320b304 into kubernetes:master Nov 30, 2016
@mtaufen mtaufen added this to the v1.5 milestone Dec 12, 2016
@mtaufen
Copy link
Contributor Author

mtaufen commented Dec 12, 2016

@jessfraz @saad-ali this needs to be cherry-picked into 1.4 and 1.5

@jessfraz jessfraz mentioned this pull request Dec 12, 2016
Merged
@mtaufen mtaufen mentioned this pull request Dec 12, 2016
Merged
@jessfraz jessfraz added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-none Denotes a PR that doesn't merit a release note. labels Dec 12, 2016
k8s-github-robot pushed a commit that referenced this pull request Dec 13, 2016
Merge pull request #38651 from jessfraz/automated-cherry-pick-of-#375…
49b4483 
…83-origin-release-1.4

Automatic merge from submit-queue

Automated cherry pick of #37583

Cherry pick of #37583 on release-1.4.

#37583: Fix nil pointer dereference in test framework
@saad-ali saad-ali added the cherry-pick-approved Indicates a cherry-pick PR into a release branch has been approved by the release branch manager. label Dec 14, 2016
k8s-github-robot pushed a commit that referenced this pull request Dec 15, 2016
Merge pull request #38654 from mtaufen/automated-cherry-pick-of-#3758…
0e97903 
…3-upstream-release-1.5

Automatic merge from submit-queue

Automated cherry pick of #37583

Cherry pick of #37583 on release-1.5.

#37583: Fix nil pointer dereference in test framework
@k8s-cherrypick-bot
Copy link

Commit found in the "release-1.5" branch appears to be this PR. Removing the "cherrypick-candidate" label. If this is an error find help to get your PR picked.

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

@Random-Liu Random-Liu Random-Liu left review comments

Assignees

@jessfraz jessfraz

@Random-Liu Random-Liu

@saad-ali saad-ali

Labels
area/test cherry-pick-approved Indicates a cherry-pick PR into a release branch has been approved by the release branch manager. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/bug Categorizes issue or PR as related to a bug. lgtm "Looks good to me", indicates that a PR is ready to be merged. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Milestone
v1.5
Development

Successfully merging this pull request may close these issues.
7 participants
@mtaufen @Random-Liu @k8s-ci-robot @k8s-github-robot @k8s-cherrypick-bot @jessfraz @saad-ali