Improve Docker dependencies code #466
Conversation
dgageot
force-pushed
the
paths
branch
6 times, most recently
from
141f837 to
b1ba745
Compare
dgageot
force-pushed
the
paths
branch
6 times, most recently
from
bc766c5 to
5d7fe29
Compare
Signed-off-by: David Gageot <david@gageot.net>
Signed-off-by: David Gageot <david@gageot.net>
pkg/skaffold/docker/context_test.go
Outdated
| os.Mkdir(filepath.Join(tmpDir, "files"), 0750) | ||
| ioutil.WriteFile(filepath.Join(tmpDir, "files", "ignored.txt"), []byte(""), 0644) | ||
| ioutil.WriteFile(filepath.Join(tmpDir, "files", "included.txt"), []byte(""), 0644) | ||
| ioutil.WriteFile(filepath.Join(tmpDir, ".dockerignore"), []byte("**/ignored.txt"), 0644) |
There was a problem hiding this comment.
Can we add a test case for #386.
Add a line to .dockerignore with "alsoignored.txt" and create a file alsoignored.txt.
There was a problem hiding this comment.
I'll do that, thanks for the idea!
| @@ -159,7 +159,7 @@ func TestGetDockerfileDependencies(t *testing.T) { | |||
| description: "add dependency", | |||
| dockerfile: addDockerfile, | |||
| workspace: "docker", | |||
| expected: []string{"docker/Dockerfile", "docker/nginx.conf"}, | |||
| expected: []string{"Dockerfile", "nginx.conf"}, | |||
There was a problem hiding this comment.
This is worry some for me. I don't have a lot of context, but earlier GetDockerfileDependencies used to return absolute paths.
Now you dont. Would this not break the workflow.
There was a problem hiding this comment.
It shouldn't but yes, that's a big change. One that's needed
There was a problem hiding this comment.
We need to keep things as relative as possible. Each time we switch to absolute paths, it's a mess
There was a problem hiding this comment.
yes i agree absolute paths are a mess.
Will take one more look.
Signed-off-by: David Gageot <david@gageot.net>
|
@tejal29 I've added the test. Regarding the paths, I spend quite some time trying to fix our path madness and keeping relative paths as much as possible was the best solution I found. There's more work to be done on the kubectl deployer side. |
Fixes #386
Also Fixes #462