fix port issues for CONTAINER_HOST #16526
Conversation
| port, err := strconv.Atoi(_url.Port()) | ||
| if err != nil { | ||
| return nil, err | ||
| port := 22 |
There was a problem hiding this comment.
You could read /etc/services for this, but I think this is fine.
LGTM
There was a problem hiding this comment.
hesitant since there are a bunch of different ssh related lines in there it seems but I can do this if we want
| return nil, err | ||
| port := 22 | ||
| if _url.Port() != "" { | ||
| port, err = strconv.Atoi(_url.Port()) |
There was a problem hiding this comment.
sure, but I will leave the if since atoi returns 0 on failure
|
one NIT ... else LGTM |
|
/approve |
openshift-ci
bot
added
the
do-not-merge/hold
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: cdoern, rhatdan The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
bot
added
the
approved
cdoern
force-pushed
the
containerHost
branch
from
5c3a8c0 to
94018d1
Compare
pkg/bindings/connection.go
Outdated
| return nil, err | ||
| port := 22 | ||
| if _url.Port() != "" { | ||
| port, _ = strconv.Atoi(_url.Port()) |
There was a problem hiding this comment.
Why is the error ignored? This will set the port to 0 on errors.
There was a problem hiding this comment.
ok I will add the error check back in
test/e2e/system_connection_test.go
Outdated
| cmd = exec.Command("export", "CONTAINER_HOST", fmt.Sprintf("ssh://%s@localhost", u.Username)) | ||
| _, err = Start(cmd, GinkgoWriter, GinkgoWriter) | ||
| Expect(err).ToNot(HaveOccurred()) |
There was a problem hiding this comment.
export and unset are not commands, they are shell built-ins.
you need os.Setenv(0 and os .Unsetenv().
But I don't see how this tests anything "--connection", "QA" should overwrite the env var, no?
There was a problem hiding this comment.
ah yes, I knew I was doing that part wrong! As for the --connection, not sure if this makes a difference as I think the env var has priority but just in case I will try without it
cdoern
force-pushed
the
containerHost
branch
from
94018d1 to
66174f9
Compare
test/e2e/system_connection_test.go
Outdated
| _, err = Start(cmd, GinkgoWriter, GinkgoWriter) | ||
| Expect(err).ToNot(HaveOccurred()) | ||
|
|
||
| err = os.Unsetenv("CONTAINER_HOST") |
There was a problem hiding this comment.
this needs to be defer os.Unsetenv("CONTAINER_HOST") right after the setenv call, otherwise you will leak the env on test errors.
if no port is specified for an ssh style url, default to 22 resolves containers#16509 Signed-off-by: Charlie Doern <[email protected]>
cdoern
force-pushed
the
containerHost
branch
from
66174f9 to
14ef6a9
Compare
|
/lgtm |
|
/hold cancel |
openshift-ci
bot
removed
the
do-not-merge/hold
github-actions
bot
added
the
locked - please file new issue/PR
if no port is specified for an ssh style url, default to 22
resolves #16509
Signed-off-by: Charlie Doern [email protected]
Does this PR introduce a user-facing change?