-
Notifications
You must be signed in to change notification settings - Fork 38.7k
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
move from daemon_restart.go to framework/util.go #45423
Conversation
@k8s-bot test this |
@k8s-bot bazel test this |
test/e2e/framework/util.go
Outdated
@@ -3835,7 +3844,7 @@ func IssueSSHCommandWithResult(cmd, provider string, node *v1.Node) (*SSHResult, | |||
host := "" | |||
for _, a := range node.Status.Addresses { | |||
if a.Type == v1.NodeExternalIP { | |||
host = a.Address + ":22" | |||
host = fmt.Sprintf("%v:%v", a.Address, sshPort) |
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.
seems orthogonal to your change.
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.
It is. But using Sprintf vs. string addition is a more common pattern from what I've seen, and I was on that line of code. I can change it back if it matters.
@liggitt @timothysc I can't discover the bazel build failure. All I see is this (each time I've re-run): |
Issue with prow. |
test/e2e/framework/util.go
Outdated
@@ -101,6 +101,8 @@ import ( | |||
testutils "k8s.io/kubernetes/test/utils" | |||
) | |||
|
|||
const sshPort = "22" |
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.
push this down to const ( ... )
block on L106?
Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please follow instructions at https://github.com/kubernetes/kubernetes/wiki/CLA-FAQ to sign the CLA. It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
@K8s test this |
@k8s-bot unit test this |
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.
net.JoinHostPort instead of sprintf
// NodeExec execs the given cmd on node via SSH. Note that the nodeName is an sshable name, | ||
// eg: the name returned by framework.GetMasterHost(). This is also not guaranteed to work across | ||
// cloud providers since it involves ssh. | ||
func NodeExec(nodeName, cmd string) (SSHResult, error) { |
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.
Is this a move? I don't see it being removed from somewhere else
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.
Duh! Thanks. Somehow daemon_restart.go didn't get commited.
test/e2e/framework/util.go
Outdated
// eg: the name returned by framework.GetMasterHost(). This is also not guaranteed to work across | ||
// cloud providers since it involves ssh. | ||
func NodeExec(nodeName, cmd string) (SSHResult, error) { | ||
return SSH(cmd, fmt.Sprintf("%v:%v", nodeName, sshPort), TestContext.Provider) |
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.
Not thrilled with exporting package functions that depend on global vars
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.
Ok. I'm open to something better. I see this type of pattern where an exported func calls an internal func, adding args to the call. Not sure if the added args are global or not. Is this a deal breaker?
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.
I won't block on it, but it seems like increased surface area without a whole lot of benefit
I will let @liggitt review this. Once he's happy with it 👍 |
go ahead and squash to a single commit, then LGTM |
@k8s-bot unit test this |
Thank you @liggitt. I need an /approve and /lgtm. |
@k8s-bot test this |
@k8s-bot pull-kubernetes-federation-e2e-gce test this |
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jeffvance, liggitt
Needs approval from an approver in each of these OWNERS Files:
You can indicate your approval by writing |
Automatic merge from submit-queue |
What this PR does / why we need it:
Moves the func
nodeExec
from daemon_restart.go to framework/util.go. This is the correct file for this func and is a more intuitive pkg for other callers to use. This is a small step of the larger effort of restructuring e2e tests to be more logically structured and easier for newcomers to understand.cc @timothysc @copejon