Use Docker parser when manipulating Dockerfiles #1103
Conversation
0xmichalis
changed the title
pkg/build/builder/docker.go
Outdated
There was a problem hiding this comment.
why not just strings.HasPrefix(strings.ToUpper(line),strings.ToUpper(cmd)) ?
There was a problem hiding this comment.
Cool, updating it...
|
No testcase for dockerfiles with trailing backslashes? |
|
regarding the thread about validating the output, i'm not sure it's necessary given that the next thing we're going to do with the file is "docker build" it so if it's invalid, we'll get the error there... us checking it beforehand doesn't seem to buy us much.... @soltysh ? |
Thats' why I have the |
|
@bparees please review the latest changes and if you are okay with them, I'll squash the pr into two separate commits and rebase into current master |
0xmichalis
changed the title
|
lgtm. @smarterclayton any concern w/ us pulling in the upstream docker change here? |
|
Let me look
|
|
@ncdc because it's pulling in upstream docker changes? |
|
My understanding is we UPSTREAM for things we want in our vendored copy of Kubernetes but that either a) aren't actually committed upstream yet, or b) are committed upstream, but we haven't rebased them in yet. See https://github.com/openshift/origin/blob/master/HACKING.md#3-cherry-pick-upstream-changes-pushed-to-the-origin-repo |
|
this is the latter case, just that upstream in this case is docker not k8s. |
|
The point I'm trying to make is we aren't rebasing Docker, so having a commit with UPSTREAM that's not related to Kubernetes might lead to confusion when someone does a Kubernetes rebase. |
|
got it. but it seems like we have a similar problem that needs to be solved for docker rebases. maybe a slightly different keyword is in order. |
|
Sure. But if all we're doing is running |
|
IMHO marking a commit as UPSTREAM or bump or whatever is nothing compared to the merge commits cluttering the history logs right now but that's another story |
|
@Kargakis it's all about what is greppable. |
|
Yeah, part of our rebase process is double checking the bumps, so grepability is critical.
|
My rationale here was as follows. Think of a use case where user provides us with valid Dockerfile but after modification we spit one that will not run build correctly, because of Dockerfile validation. This gets then reported back to user and he can't do nothing with it other than filling a bug. What I'm targeting here is we need to ensure that whatever comes out of our replace function must be valid Dockerfile implying the input was also valid. If we're tightening here the parsing let's go one step further and make sure that whatever comes out is 100% valid or we're getting replacement error, IMHO. |
|
@Kargakis would you mind modifying the UPSTREAM commit to say bump instead? |
No problem, done |
|
@Kargakis thanks. I did just notice that the Docker commit you're pulling in isn't from a tag. @smarterclayton do you want the Docker commit to come from a tag, or is this ok? |
|
@soltysh they can't do anything about it either way is my point, and it's going to get caught either way. but sure, catching the error a little sooner where we can control the reported error message is not a bad idea. |
|
@bparees since we allowed those parameters then it should produce workable output, otherwise we failed at the parameter check step, which is other way of seeing this things. |
Aren't the existing tests accomplishing that? |
|
@Kargakis what @soltysh is suggesting isn't a new test case, it's output validation. If someone provides an image name like "my image" then the final Dockerfile would contain "FROM my image" which presumably is invalid. (this means we should probably have more validation on what is valid for the image field, if we don't already, but the point is we can end up with an invalid Dockerfile in a variety of ways and @soltysh thinks we should check for that rather than punt to docker build to catch the error) |
Ok, got it. Isn't it kind of irrelevant to my changes though? The functionality implemented here replaces Dockerfiles commands, while validating the image field is another story. |
|
Bitten by merge conflicts but resolved them yesterday. Also rebased on top of the current master. |
|
it's not about validating just the image field, it's about validating the whole file to confirm we haven't accidentally corrupted it during our modification process (eg suppose there's some bug in your replacement logic). so after doing the replace, you should confirm the new Dockerfile is valid. |
Ok, I am going to add that test I had in mind ie. parsing the old and new Dockerfiles and compare their ASTs. I will also add tests on |
|
Just added tests for comparing the Abstract Syntax Trees of the original and the edited Dockerfiles. Later I will write tests for |
5 participants
Add one more layer of parsing for safety by using the Docker
parser to verify the valid occurrence of a Dockerfile command.