refactor: Refactor -p/--publish flag test for run command #39
Conversation
Signed-off-by: Vishwas Siravara <vsiravara@gmail.com>
| for i := 0; i < maxRetry; i++ { | ||
| conn, err = net.Dial(network, address) | ||
| // #nosec G107 // it does not matter if url is not a constant for testing. | ||
| resp, err := http.Get(url) |
There was a problem hiding this comment.
nit: The default http client does not have timeout, but we may want to set one (maybe just 3~5 seconds since the traffic is only on localhost) as a good practice. More details: https://stackoverflow.com/a/25344458/6355435
| gomega.Expect(b).To(gomega.Equal([]byte(want))) | ||
| gomega.Expect(conn.Close()).To(gomega.Succeed()) | ||
| gomega.Expect(resp.StatusCode).To(gomega.Equal(want)) | ||
| gomega.Expect(resp.Body.Close()).To(gomega.Succeed()) |
There was a problem hiding this comment.
nit: Usually resp.Body.Close() is defered right after checking that the err from the request is not nil. This ensures that the response body is closed no matter what happens later.
There was a problem hiding this comment.
nit: Maybe we'd like to make this a fix: instead of a refactor:. Rationale: we want this to trigger a release (I'm aware that there's already a release-please PR, but theoretically we want this PR to trigger one if there's no existing one), and it's more of a fix essentially (i.e., it "fixes" runfinch/finch#196).
Re. how to fix it, see the description of runfinch/finch#208 for an example of BEGIN_COMMIT_OVERRIDE and END_COMMIT_OVERRIDE.
|
Made a follow up PR to address comments on this merged pull request. |
) Issue #, if available: *Description of changes:* Code review fix for #39 *Testing done:* Yes - [X] I've reviewed the guidance in CONTRIBUTING.md #### License Acceptance By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license. Signed-off-by: Vishwas Siravara <siravara@amazon.com>
Issue #, if available:
runfinch/finch#196
Description of changes:
Use nginx server for testing port publish(
-p/--publish) flag in the run command instead of busybox netcat. This is due to the fact that the cni gateway in rootful makes a request to the netcat server running in the container and closes the connection which causes netcat to exit before a client on the host can make a connection to it.Testing done:
Yes.
License Acceptance
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
BEGIN_COMMIT_OVERRIDE
fix: Switch from netcat to nginx for testing publish flag in run command
END_COMMIT_OVERRIDE