-
Notifications
You must be signed in to change notification settings - Fork 54
Improve capabilities for multistage Dockerfiles #58
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
Conversation
|
I knew I was going to like having you on my team @sosiouxme |
|
Travis CI test fails due to lack of #59. |
|
That's what I get for skipping the python 2.6 testing. Got an actual test failure to fix. |
|
@sosiouxme Python 2.6 only came out 10 years ago. Clearly it should be still tested :P |
dockerfile_parse/parser.py
Outdated
| for stage in range(len(froms)-2, -1, -1): # e.g. 0 for single or 2, 1, 0 for 3 stages | ||
| start, finish = froms[stage], froms[stage+1] | ||
| linenum = start['endline'] + 1 if at_start else finish['startline'] | ||
| df_lines[linenum:linenum] = [_endline(line) for line in lines] |
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.
Nit: we're calculating _endline() repeatedly for the same values. Can we move that out of the loop?
tests/test_parser.py
Outdated
|
|
||
| parents = dfparser.parent_images | ||
| for img in FROM: | ||
| assert img in parents |
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.
How come we're not specifying the order they should appear?
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.
Thanks, I didn't update this with the implementation
| matches = [index for index, text in enumerate(df_lines) if text == anchor] | ||
| if not matches: | ||
| raise RuntimeError("Cannot find line in the build file:\n" + anchor) | ||
| anchor = matches[-1] |
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.
Oh, the last match? Can we have a test case that checks this?
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.
Modified test_add_lines_at to cover this
| if instr['instruction'] != 'FROM': | ||
| continue | ||
| image, _ = image_from(instr['value']) | ||
| if image is not None: |
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.
I don't think we have testing that covers this not being the case.
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.
Added one. And actually setting parent_images would have been broken in that case (I fixed it). Well, the build would be broken anyway, but we can at least do no further harm.
Scope most existing properties to only the final stage. Add parent_images property for getting and setting a list of images from the stages. Introduce add_lines and add_lines_at methods for simpler manipulation. Introduce is_multistage property for convenience.
|
Updated release notes draft:
|
Introduce
add_linesandadd_lines_atmethods for simpler manipulation.Scope most existing properties to only the final stage.
Add
parent_imagesproperty for getting and setting a list of images from the stages.Introduce
is_multistageproperty for convenience.@twaugh @lcarva @mlangsdorf @rcerven PTAL
/cc @peterkline @adammhaile does this help...