Feature/heredocs #202
Feature/heredocs #202
Conversation
parse_test.go
Outdated
| StartLine: 1, | ||
| EndLine: 5, | ||
| Flags: []string{}, | ||
| Value: []string{"content 1\n", "content 2\n"}, |
There was a problem hiding this comment.
where did /dest go? this doesn't seem right
There was a problem hiding this comment.
I totally agree, but unfortunately that is how buildkit is parsing that case. If you look at the sample test case they have at https://github.com/moby/buildkit/blob/master/frontend/dockerfile/parser/parser_heredoc_test.go line 67, then the expected value at 204, the /dest does disappear. I am not sure why they are doing this, but since we are not trying to improve on their parser but just support what they do, this is the way our test case is behaving as well. At most, we could think about putting it in some manner in the Value field, which I was kinda doing before, but changes in the heredoc substructure should depend on changes they make.
There was a problem hiding this comment.
the whole point of this is to allow one to get the whole structural syntax by improving on the parser
There was a problem hiding this comment.
Not really. The whole point is to bring parity with the current version of the docker parser in buildkit, which has heredocs support. Since this library is just a wrapper around that, the best we can do is to wrap around what that parser is doing. If that parser has bugs or shortcomings, the proper place to fix it is over there.
There was a problem hiding this comment.
That being said, I realized that the contents of the Value field now for the heredocs do not make much sense, given the new Heredocs structure and the fact that the Heredocs contents are now in there. I pushed a new change where I modified the Value field to go back to how it works for all command, which for the Heredocs case means that it will parse the first line of the command, including the names of the heredocs and anything else in that line, like the "/dest" param in that line. The Heredocs substructure will work as the attachment with the contents of the heredocs, which is how buildkit does it and how it is probably used.
tests/dockerfile_test.py
Outdated
| ), | ||
| dockerfile.Command( | ||
| cmd='CMD', sub_cmd=None, json=True, value=('echo', '☃'), | ||
| start_line=2, end_line=2, original='CMD ["echo", "☃"]', flags=(), | ||
| heredocs=(), |
There was a problem hiding this comment.
this indicates an api breaking change
There was a problem hiding this comment.
Fair point, but this is due to the api breaking change buildkit did by adding the new herdocs structure, so we are consistent with their changes. If there is a way to pass a default value to the go-to-python constructor for the Command, I could do that to avoid this external change, but I have not looked into that yet. If you feel this is important, I can try to add a default empty value. However, the current behavior is consistent with, for example, the flags argument, which is provided an empty value when no flags are used.
There was a problem hiding this comment.
I wouldn't have pointed it out if I didn't want you to fix it
There was a problem hiding this comment.
Right, which is why I replied by pointing out the pros and cons. Pushed a new version that avoids breaking the API.
pylib/main.go
Outdated
|
|
||
| pyFileDescriptor := C.PyLong_FromLong(C.long(heredoc.FileDescriptor)) | ||
|
|
||
| pyContent := stringToPyOrNone(heredoc.Content) |
There was a problem hiding this comment.
I don't think this should be OrNone ?
pylib/main.go
Outdated
| C.Py_DecRef(ret) | ||
|
|
||
| } |
There was a problem hiding this comment.
please review your PR and remove any unnecessary changes like this one (there are several)
There was a problem hiding this comment.
Cleaned up a couple of lines like this one
pylib/support.c
Outdated
| PyObject* defaults = PyTuple_New(1); | ||
| PyObject* default_heredoc = PyTuple_New(0); | ||
| PyTuple_SetItem(defaults, 0, default_heredoc); |
There was a problem hiding this comment.
pretty sure this can be done simpler with BuildValue ?
asottile
force-pushed
the
feature/heredocs
branch
from
310cbac to
c33a0ef
Compare
Added support for properly parsing heredocs in dockerfiles. Addresses #174 .