-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Resolve relative paths to absolute paths in command line arguments #736
Conversation
4a4b0ff
to
2818b3e
Compare
cmd/executor/cmd/root.go
Outdated
var err error | ||
absp := *p | ||
if *p, err = filepath.Abs(*p); err != nil { | ||
return errors.Wrapf(err, "Couldn't resolve relative path %s to an absolute path", p) |
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.
On this line, p
should be *p
I'll push a fix asap.
2818b3e
to
ad587f6
Compare
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.
Hey @huguesalary this is looking good -- would you mind adding a test for this?
@priyawadhwa Sure thing, though I am unclear as to where exactly I should add my test files. Would you mind pointing this out to me? |
ad587f6
to
c7104e7
Compare
Also, I force pushed again because
|
For sure -- we have an integration test folder with all of our integration tests. for example, this is the test for caching. You could add another test here to build a kaniko image with relative paths passed in, using the context directory for the context and using a Dockerfile in the dockerfiles directory to build. If the build completes succesfully, we can say the test has passed. Let me know if you have any more questions! |
@priyawadhwa Thanks! Should I create a new Dockerfile or should I follow the caching test model and write a test that builds every Dockerfile available in the test directory? |
Creating a new Dockerfile or using one of the existing ones and testing once should be sufficient. |
The executor accepts a few arguments dockerfile, context, cache-dir and digest-file that all represent paths. This commits allows those paths to be relative to the working directory of the executor. Fixes GoogleContainerTools#732 GoogleContainerTools#731 GoogleContainerTools#675
c7104e7
to
4327e9b
Compare
@priyawadhwa I just added the integration test, though to be honest, I'm not entirely sure how correct it is. It took me a few hours to figure out how exactly I'm supposed to write the test. It seems like there's a lot of repeated code in there that could probably be refactored to be more generic and make it easier to write tests. I might take a stab at it, though I'm not too familiar with the codebase. |
@@ -227,6 +230,37 @@ func resolveSourceContext() error { | |||
return nil | |||
} | |||
|
|||
func resolveRelativePaths() 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.
@huguesalary Thank you for your contributions. This looks really good. However can we please some unit test for this?
Can you refactor resolveRelativePaths()
to accept opts? That ways we can easily write unit tests
func resolveRelativePaths() error { | |
func resolveRelativePaths(opts config.KanikoOptions{}) error { |
Now, you can creates tests where
func TestResolveRelativePaths(t *testing.T) {
tests := []struct {
description string
opts config.KanikoOptions
expected config.KanikoOptions
shdErr bool
}{
{
description: "dockerfile path is absolute",
opts: config.KanikoOptions{
DockerfilePath: "/abs/dockerfile",
},
expected: config.KanikoOptions{
DockerfilePath: "/abs/dockerfile",
},
}
}
for _, tt := range tests {
actual := resolveRelativePaths(tt.opts)
t.CheckDeepEqualAndError(tt.shdErr, actual, tt.expected, tt.opts)
}
}
Hi @tejal29 sure thing I'll add your suggestion and some unit tests |
Hi, thank you for your contribution to this great repo. Just to put a note that I suspect this PR broke my build this morning: I used Build logs before:
Build logs after:
My config was incorrect. After I change the config to I would be appreciate if we could make releases backward compatible, even if the old behavior is incorrect because maybe there are people depend on it. Or at least put a note in the changelog. If this PR did not cause the broken build as I suspected: I am sorry, please ignore this. Thanks! |
The executor accepts a few arguments
--dockerfile
,--context
,--cache-dir
and--digest-file
that all represent paths. This commits allows those paths tobe relative to the working directory of the executor.
This code assumes
context
is an actual path and not a URL, which it should be when the newresolveRelativePaths()
function introduced by this commit is called.Fixes #732 #731 #675