fetcher: fix dependency issue, new test TestImageDependencies + disco #1822
Conversation
|
I updated the scenario to the exact one in #1752 and I can reproduce it:
|
ada3898 to
262eb02
Compare
|
Rebased with the last changes from appc/spec#551 |
| // Actually, httptest.Start() is broken because it waits forever, so | ||
| // I have to use httptest.StartTLS(). | ||
| // So, patching appc/spec to use InsecureSkipVerify: true | ||
| serverURL := flag.Lookup("httptest.serve") |
There was a problem hiding this comment.
This change probably won't be necessary, when the appriopriate change in appc/spec lands, right?
There was a problem hiding this comment.
You're right. This one: "discovery: add port parameter" appc/spec#110
There was a problem hiding this comment.
Ah, no. It probably can be dropped, when we have the discovery over custom port implemented in spec.
262eb02 to
2a234b6
Compare
|
@krnowak branch updated |
| ep, err := discoverApp(app, f.headers, true) | ||
|
|
||
| var insecure discovery.InsecureOption | ||
| // FIXME: why do we always accept http? |
There was a problem hiding this comment.
Because we are secure by default! Oh, wait...
;)
There was a problem hiding this comment.
Jokes aside, we should add an insecure flag for allowing http connections and use it here. There was even an issue about this, right?
There was a problem hiding this comment.
and added a comment referencing the issue
2a234b6 to
1c7ebe8
Compare
|
Thanks for the review. Branch updated. I also fixed some comments. |
1c7ebe8 to
44527d8
Compare
|
depends on:
|
44527d8 to
eca48b8
Compare
5047afc to
01a0017
Compare
|
The new appc/spec has been vendored in #1861. Labels "do not merge" and "dependency/appc spec" removed. Rebased and ready for review. |
| // This means this test must: | ||
| // - use https only | ||
| // - ignore tls errors with --insecure-options=tls | ||
| serverURL := flag.Lookup("httptest.serve") |
There was a problem hiding this comment.
Maybe handle the case, when serverURL is nil. Like panic when it is nil, with some useful message instead of getting a nil dereference error.
There was a problem hiding this comment.
I mean - the serverURL variable is accessed below without any prior nil checks.
|
Some nits, otherwise LFAD. |
01a0017 to
a6c113d
Compare
|
Updated. |
| go serverHandler(t, server) | ||
| return server | ||
| } | ||
|
|
||
| func serverHandler(t *testing.T, server *taas.Server) { |
There was a problem hiding this comment.
The serverHandler function needs to be moved too.
|
More nits. |
a6c113d to
38604da
Compare
|
Updated again |
| if err := ioutil.WriteFile(tmpManifest.Name(), []byte(i F438 mg.manifest), 0600); err != nil { | ||
| panic(fmt.Sprintf("Cannot write to temp manifest: %v", err)) | ||
| } | ||
| defer os.Remove(tmpManifest.Name()) |
There was a problem hiding this comment.
This defer should be placed before ioutil.WriteFile.
|
One final nit. Sorry for not spotting it earlier. :) |
Test inspired by rkt#1752
38604da to
cb99078
Compare
|
LFAD. |
fetcher: fix dependency issue, new test TestImageDependencies + disco
fetch: allow duplicate dependencies in the dependency tree
Fixes #1752
Includes a new test TestImageDependencies that could reproduce the bug. The test run an image discovery on a local https server (port 443) with the appropriate insecure option.
I cannot run the test on http because httptest waits forever when using
httptest.Start() on a non-random port, see:
https://golang.org/src/net/http/httptest/server.go?s=2768:2792#L112
And I have to use the non-random port 443: I need standard https port because
of #375 (comment)