allow dockerfiles to be anywhere #178
Conversation
| @@ -12,8 +12,10 @@ import ( | |||
|
|
|||
| var multilineCommentMarginRegexp = regexp.MustCompile(`(?m)^\s*[*]*[ \t]*`) // we need to use [ \t] instead of \s, because \s includes newlines in (?m) mode. | |||
|
|
|||
| const js = core.LanguageId("javascript") | |||
There was a problem hiding this comment.
I moved this into the struct a while back and just use Language.ID (since it's exported and IMO clearer what it is).
pkg/infra/pulumi_aws/deploylib.ts
Outdated
| const image = this.sharedRepo.buildAndPushImage({ | ||
| context: `./${execUnitName}`, | ||
| context: `./${execUnitName}/${dockerfilePath}`, |
There was a problem hiding this comment.
I think this needs to be
{
context: `./${execUnitName}/${path.dirname(dockerfilePath)}`,
dockerfile: `./${execUnitName}/${dockerfilePath}`,
}Though I'm not sure if using that context is correct versus using ./${execUnitName}.
There was a problem hiding this comment.
so i printed out what my code has for the context and its ./main/src which would be correct
There was a problem hiding this comment.
we can leave context at the root of the exec unit if thats what you mean
There was a problem hiding this comment.
the path is already filepath.Dir in go so it doesnt include the file name
There was a problem hiding this comment.
Hm, isn't that wrong then? What if your dockerfile was build/myservice.Dockerfile? At the least the field name doesn't match its content.
There was a problem hiding this comment.
i might be a bit confused by what you mean. Doesnt it assume that there is 1 and only 1 dockerfile within the context and it will use that? if it was build/myservice.Dockerfile then we would point to {execunit}/build/myservice.Dockerfile which i would think is correct
There was a problem hiding this comment.
The (see below). I don't believe it does any sort of searching. Additionally, by sending the context of file parameter defaults to Dockerfile, but I don't believe it's relative to the context but rather the current working directory from when it's run{execunit}/build that would mean it couldn't include any source files in {execunit}/src (or similar, outside of the build directory).
nm on the first (default), part:
Usage: docker build [OPTIONS] PATH | URL | -
[...]
-f, --file string Name of the Dockerfile (Default is 'PATH/Dockerfile')
There was a problem hiding this comment.
theres no way we can understand the context without parsing the Dockerfile, so are you suggesting just leaving it at the root exec unit so we can encompass everything? If so then thats fine with me and similarly is the question, does the --file parameter only look for files named Dockerfile or would it find any Dockerfile?
There was a problem hiding this comment.
theres no way we can understand the context without parsing the Dockerfile
Not only that, but even that might not be able to tell you enough (though in many circumstances we could make a guess based on any ADD/COPY statements). This would have to be a directive to support all the different edge cases.
are you suggesting just leaving it at the root exec unit so we can encompass everything?
Yeah, for now until we get some more intelligence built in.
does the --file parameter only look for files named Dockerfile or would it find any Dockerfile?
I'm relatively certain it only finds the file named Dockerfile. Most non-trivial build processes I've seen use explicit -f path/to/dockerfile for this reason.
There was a problem hiding this comment.
ok i can go make these updates then, will have a new revision out today
Sanitizes AWS resource names in deploylib and related IAC files
jhsinger-klotho
force-pushed
the
dockerfile
branch
from
2596fc9 to
20a7dfb
Compare
|
|
the branch got a bit messed up, so i had to rebase to fix conflicts with main so the changes from the last commit should just be to address the comments, but hard to see now |
• Does any part of it require special attention?
• Does it relate to or fix any issue? closes #74
we currently require dockerfiles to be at the klotho root path. With this change if a custom dockerfile is provided we will obey the location it is provided at in regards to the root path of the klotho invocation.
In order to make this work we have to add any annotated file as a sourcefile to prevent it from getting pruned /~https://github.com/klothoplatform/klotho-pro/pull/50 is the pro request for the same.
to standardize this logic in pulumi, i created a createImage function. We then store the exec unit images in a map to be retrieved by the other functions so we dont have to recreate this in 4 places and muddy the params to those other functions
Standard checks