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

ci: Docker plugin #440

Merged
merged 2 commits into from
Jan 16, 2017
Merged

ci: Docker plugin #440

merged 2 commits into from
Jan 16, 2017

Conversation

talex5
Copy link
Contributor

@talex5 talex5 commented Jan 13, 2017

This plugin provides the ability to build Docker images, including builds where one built image is used as the base image for another. This is needed to test DataKit itself (needed to make #439 go green!).

Note that @avsm has a more sophisticated Docker plugin at https://github.com/avsm/mirage-ci/ for when the Dockerfile is generated by the CI. This PR's plugin handles the simple case of a project which already has a hand-written Dockerfile.

Thomas Leonard added 2 commits January 13, 2017 14:31
Signed-off-by: Thomas Leonard <thomas.leonard@docker.com>
This plugin provides the ability to build Docker images, including
builds where one built image is used as the base image for another.

Signed-off-by: Thomas Leonard <thomas.leonard@docker.com>

module Image = struct
type t = {
id : string;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be useful to unify this with https://github.com/avsm/mirage-ci/blob/master/src/docker_build.mli#L14 so that we have one type for Docker images within the CI. I separated out the sha256 (which is queried after the image is built) from the textual tag, and added a human-readable string as well to describe the purpose of the file (this is probably optional).


let branch_safe_char = function
| ':' -> '-'
| x -> x
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could branch_safe_char be exposed elsewhere for other plugins to use? Or is there any reason why the results of branch shouldnt always be run through this for all plugins?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Depending on the way the branch is generated, replacing characters automatically could make the branch names non-unique. Possibly we should define some custom escaping scheme (in Irmin) and apply it everywhere.

| None -> ""
| Some from -> "-from-" ^ String.map branch_safe_char (Image.id from)
in
Printf.sprintf "docker-build-%s-of-%s%s" t.label (CI_git.hash src) from
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can be Fmt.strf for consistency

| None -> Lwt.return ()
| Some base ->
Lwt_io.with_file ~mode:Lwt_io.input path (fun ch -> Lwt_io.read ch) >>= fun contents ->
match String.cut ~sep:"\n" contents with
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to filter for String.is_prefix ~affix:"#" here as well, to account for comments before the FROM ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I think so.

Copy link
Contributor

@avsm avsm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I could easily switch my Dockerfile.t generator to use this version, by generating a file. That also has the advantage that it would make scripting easier as I could create a temporary directory for COPY commands to work as well. However, I would need sha256 image id tracking after a build...

@talex5
Copy link
Contributor Author

talex5 commented Jan 13, 2017

This one does store the sha256 after the build. It uses the somewhat hacky method of building it twice - once normally to get log output and then again with -q to get the image ID, relying on the caching to make the second build fast.

@talex5
Copy link
Contributor Author

talex5 commented Jan 16, 2017

After discussion with @avsm I'm tempted to merge this now, as the builds are broken without it. The comment thing should be supported at some point, but we might prefer to use a proper Dockerfile parser rather than hacky string matching, and the current code will give a clear error if it gets an unsupported format.

@avsm
Copy link
Contributor

avsm commented Jan 16, 2017

lgtm, I dont think this clashes with anything I'm doing. I've created ocurrent/ocaml-dockerfile#2 to track the Dockerfile parser request in the library.

@talex5 talex5 merged commit cd4f7bd into moby:master Jan 16, 2017
@talex5 talex5 deleted the docker branch January 16, 2017 12:05
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.

2 participants