Skip to content
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

Added support for url build paths #1372

Closed
wants to merge 1 commit into from
Closed

Conversation

jonaseck2
Copy link

Implements #1369
Fixes #491

Shamelessly stole url validation logic from docker.

Manual testing works fine but I've been unable to run the unit tests due to a 404 error from docker when running script/test.

@aanand
Copy link

aanand commented Apr 29, 2015

Nice. I think that path validation logic belongs in docker-py, though.

@funkyfuture
Copy link

i'm afraid it belongs to the config-validation. in #1355 the validation-strategy is to accept nothing that is not defined as being allowed.

@jonaseck2
Copy link
Author

Looks like you've already implemented url validation.

@jonaseck2
Copy link
Author

Would this fix be a candidate for a 1.2.1 release? I'm unable to use 1.2.0 due to the broken url support.

Signed-off-by: Jonas Eckerström <jonaseck@gmail.com>
@aanand
Copy link

aanand commented May 5, 2015

@funkyfuture Nonetheless, it's not specific to Compose, and should at least be a utility function under docker.utils.

@jonaseck2 Is it a regression? I don't think Compose ever supported build URLs.

@jonaseck2
Copy link
Author

@jonaseck2
Copy link
Author

Before 1.2.0 I'm assuming that compose just transparently forwarded the build path to docker and let docker validate the build path. In that sense, adding validation of the build path only limits what docker compose allows itself to pass to docker. I think it's a better idea to separate concerns between docker and compose and leave validation of argument values to docker.

Initially, my solution to the issue was to check if the build path was a file path by testing if the relative path didn't exist and the expanded path did, otherwise just send it to docker as is.

Perhaps this is a better solution after all, both specifically and for compose in general?

@funkyfuture
Copy link

Nonetheless, it's not specific to Compose, and should at least be a utility function under docker.utils.

i didn't meant to deny that. but that it's crucial to do a rehearsal of a composition before the the band goes to stage. i'm kind of traumatized by the expierence of data-loss due to a misconfiguration.

so, my remark was like a loud thinking to remind me. in the meantime i added an url-validation to the buildpath-check in #1355.

@aanand
Copy link

aanand commented May 6, 2015

I think it's a better idea to separate concerns between docker and compose and leave validation of argument values to docker.

Some validation is going to have to happen client-side, because the client needs to know whether to create a tarball or not. But docker-py is already doing that.

What if we just remove the build path validation code?

@jonaseck2
Copy link
Author

Yes, and just leave enough logic to figure out if it's a file path and apply expansion or not

@dnephin
Copy link

dnephin commented May 31, 2015

LGTM

@aanand
Copy link

aanand commented Jun 1, 2015

Could we get a unit test for invalid URLs?

@dnephin
Copy link

dnephin commented Aug 24, 2015

I believe this overlaps with #1244 which has docs and a test case

@dnephin
Copy link

dnephin commented Oct 6, 2015

An integration test and some docs (which could reference https://docs.docker.com/reference/commandline/build/) would be great.

Will need a rebase as well to update with master

@dnephin dnephin added this to the 1.5.1 milestone Nov 3, 2015
@dnephin
Copy link

dnephin commented Nov 5, 2015

I'd like to get this into the 1.5.1 release. If you don't have any time to update it, let me know, and I will rebase it and add an integration test.

@dnephin dnephin modified the milestones: 1.6.0, 1.5.1 Nov 10, 2015
@jonaseck2
Copy link
Author

Please do. I've been trying to get this done for quite some time now and I haven't found the time for it.

@dnephin dnephin modified the milestones: 1.5.2, 1.6.0 Nov 17, 2015
@dnephin dnephin mentioned this pull request Nov 19, 2015
@dnephin
Copy link

dnephin commented Nov 19, 2015

Rebased in #2430

@dnephin dnephin closed this Nov 19, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unable to fig build a github repo / works with docker build
5 participants