Refactor runBuild

[dnephin]

Fixes #242

Details about each change in the git commits

[codecov-io]

Codecov Report

Merging #294 into master will increase coverage by 0.05%.
The diff coverage is 34.84%.

@@            Coverage Diff             @@
##           master     #294      +/-   ##
==========================================
+ Coverage   53.46%   53.52%   +0.05%     
==========================================
  Files         218      220       +2     
  Lines       14642    14679      +37     
==========================================
+ Hits         7829     7857      +28     
- Misses       6327     6338      +11     
+ Partials      486      484       -2

[tonistiigi]

My takeaway from this is that the code is harder to understand as the separation points seem quite arbitrary. I don't know what is buildIDFile, buildInput, buildBuffer etc. and I would guess people even less familiar with this code wouldn't know either. Many functions raise questions about why these parameters would be needed based on the function name, or methods use private struct properties in non-obvious places. 100+ new lines.

[tonistiigi]

Progress

[tonistiigi]

Still printing io.Writer as a string, please let's not do this.

[dnephin]

Fair, We should cast it to bytes.Buffer first, and panic if it's the wrong type.

Since this is now encapsulated, it's easy to verify that the writer is the correct type when quiet.

[tonistiigi]

I don't think cast is a correct solution. For the case where it appeared it would have just meant a different error. Type safety is the main thing protecting against incorrect code changes.

[dnephin]

Sorry, by "cast", I meant "type assertion". We can check if the field is a buffer, and call buffer.String() instead.

[dnephin]

This is fixed. We are no longer printing io.Writer, and I renamed buildBuffer to buildOutputBuffer and made it more generic.

[tonistiigi]

I didn't check but would guess this breaks showing progress output on errors.

[dnephin]

I'll look into it further. It seems --stream is using these buffers differently.

[dnephin]

What is the correct behaviour when --stream --quiet are both set?

Will stream print the errors correctly to the buffer or do we need a special case like we have with just --quiet?

[dnephin]

For now the behaviour is that --stream overrides the buffer, which means that progress/output buffers will not be printed by the --quiet special case.

I'm not sure if this is correct or not, because I'm not sure how --stream handles quiet.

[dnephin]

My takeaway from this is that the code is harder to understand

This is just a first iteration. I think some of the naming could be improved quite a bit. Overall separating concerns will make code much easier to read in the long run. The existing code is completely unreadable, so I don't see how this could possibly make it worse.

the separation points seem quite arbitrary

They are far from arbitrary. The code is separated by functional groups. A big part of this runBuild was just setting up the buildCtx. This logic doesn't need to be mixed in with a dozen other concerns.

I don't know what is buildIDFile

The name seems fairly obvious to me (it's a file which stores a build ID). The implementation also makes it obvious, it's an object that is created from options.imageIDFile (a filename), and can Save(imageID), and Remove(). The comments say about as much.

How can I help make this more clear?

buildInput

This name is not great. Any suggestions for a better name? This is the input to a build. The build context and the Dockerfile.

buildBuffer

Again, the name could be improved. These are output buffers. So maybe buildOutputBuffer ?

Many functions raise questions about why these parameters would be needed based on the function name

Please comment on these where you see them so the naming can be improved. I just looked over them again and they all seem pretty logical to me.

methods use private struct properties in non-obvious places

I'm not sure what you mean by this. Please comment on the line.

100+ new lines.

Most of the new lines are new tests, blank lines, and imports.

Even if it wasn't, optimizing for fewer lines does not make for clean code. Fewer lines is great when there is duplication to remove, but otherwise it leads to less readable code.

[tonistiigi]

They are far from arbitrary.

I didn't mean that they are not correctly picked, just that this is complex code and drawing boundaries with a ruler to complex code doesn't necessarily improve it. Example: the first function setupContextAndDockerfile returns a buildInput but if a reader thinks that this is the actual input then they are mistaken. There are several places after this call that modify the properties of that structure(trust, compress etc). Stream properties are set after this function so you would think that it should be just called setupContextAndDockerfileIfNoStream but then you see that there is nothing that sets up dockerfile for a stream. Then you look at setupContextAndDockerfile implementation more carefully and discover it does set up dockerfile for a stream but not the context.

The name seems fairly obvious to me (it's a file which stores a build ID).

The obvious question is what makes this different than any other file so it needs to be wrapped like this. The answer seems to be that it is to preserve one specific error being outputted in the Save() method. From the caller, I have no idea that this error exists.

Please comment on these where you see them so the naming can be improved.

I would if I had better ideas. setupContextStream->addDirToSessionWithProgressAndBlockingBuildBufferUntilCompleted doesn't seem like a good solution.

I don't want to argue about this much as this is far from the worst things we have merged. After spending time on it I'm starting to understand what is going on, but I'm not sure if I want other people to have the same experience of repeatedly switching through a lot of files looking for the code that I know needs to be somewhere to understand it.

[dnephin]

From the caller, I have no idea that this error exists.

Exactly, that's kind of the point! The caller shouldn't be aware of any of the implementation details. It should only be concerned with the high level operation it needs to perform (save the file, or remove the file).

I'm not sure if I want other people to have the same experience of repeatedly switching through a lot of files looking for the code that I know needs to be somewhere to understand it.

I don't see how this PR changes this. Before this change there was plenty of code that was not in runBuild(). You still had to go to a bunch of files to understand everything, but understanding the logic of runBuild() was a lot more difficult because many different concerns were mixed up together, and the size of the function meant there were many state changes happening to a single variable.

By splitting it up further we get a better sense of the high level work done as part of "build". You shouldn't need to understand the implementation of every function to understand what build does. Every line in runBuild that isn't a high level operation is cognitive load that someone has to take on just to understand the basics of what happens on build. This changes gets us a bit closer to function that reads cleanly. It's still too large, but it's getting closer at least.

[vdemeester]

ping @dnephin @tonistiigi what is the status here ?

[dnephin]

The build command still needs to be refactored.