Map Commands instead of using them as a slice #10914
Conversation
|
Can you please sign your commits following these rules: https://github.com/docker/docker/blob/master/CONTRIBUTING.md#sign-your-work The easiest way to do this is to amend the last commit: $ git clone -b "map-commands" git@github.com:kargakis/docker.git somewhere
$ cd somewhere
$ git commit --amend -s --no-edit
$ git push -fThis will update the existing PR, so you do not need to open a new one. |
|
@GordonTheTurtle done! |
|
@Kargakis I'm not sure what this PR does. You setup a map but then never actually modified any code to use it as a map. |
|
@Kargakis please revert the change in docker_cli_build_test.go - we still need to use the old gofmt syntax on those loops for a while. |
The modification helps anyone do the following:
In Docker, you just use that slice in the test I modified to be used as a map and nowhere else. If I am missing it somewhere, please point it out so I can fix it. |
|
@Kargakis yes I understand how a map can help speed things up, but my points is that if we never actually do that then I'm not sure I see the point in setting up a map - especially when our one and only use of that structure is to loop over it instead of a map-lookup. If you look at https://github.com/docker/docker/blob/master/builder/evaluator.go#L61 and https://github.com/docker/docker/blob/master/builder/evaluator.go#L284 you'll see that we do use a map for the exact reason you say. |
Yep, I have seen that map, but it's unexported so it's of no use to people outside Docker. The same pr that made it unexported, added the slice in question. |
|
yes and it was created because we needed to "loop" over the commands. |
But you don't actually use it except that test. Btw, you can loop over a map as well (even though a Go map returns the keys/values in random) |
|
right but that's the only reason it was created. By point is that if we had some other use for it (as a map) then I think this change is worth it. |
|
let me clarify.... I'm not -1'ing this. I was just pointing out that this change doesn't really help anything within Docker right now - meaning its not addressing a pain-point. I understand you're suggesting we do it because we may need a quick look-up at some point in the future. I guess you could say I'm +0 on it. |
I made this pr because on OpenShift I am working on manipulating Docker files, at some point I want to verify that a Docker command is valid and the best way to do this is to have an operation from the Docker project that will do that for me instead of manually creating the map and keeping it on sync every time something changes upstream. |
|
Ah, I didn't realize that - thanks a ton for the info. In that case, can you fix the jenkins issue? #10914 (comment) |
The most obvious use case is when one wants to make sure as fast as possible that a command is a valid Dockerfile command. Signed-off-by: kargakis <kargakis@users.noreply.github.com>
Yup, fixed |
|
LGTM |
1 similar comment
|
LGTM |
Map Commands instead of using them as a slice
The most obvious use case is when one wants to make sure as fast
as possible that a command is a valid Dockerfile command.