Page MenuHomePhabricator

Escape docker output
ClosedPublic

Authored by thcipriani on Jul 6 2017, 9:27 PM.

Details

Maniphest Tasks
T167999: Escape Blubber config values when compiling to Dockerfile
Reviewers
dduvall
mmodell
Group Reviewers
Release-Engineering-Team
Commits
rGBLBR3ad2c475d963: Escape docker output
Patch without arc
git checkout -b D705 && curl -L https://phabricator.wikimedia.org/D705?download=true | git apply
Summary

This adds a new DockerInstruction interface that can be implmented by
different docker instructions. DockerInstruction implements a compile
method that returns escaped output for use in a dockerfile.

Fixes T167999

Diff Detail

Repository
rGBLBR Blubber
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

thcipriani created this revision.Jul 6 2017, 9:27 PM
Restricted Application added a reviewer: mmodell. · View Herald TranscriptJul 6 2017, 9:27 PM
Restricted Application added a reviewer: Release-Engineering-Team. · View Herald Transcript
Restricted Application added a project: Release-Engineering-Team. · View Herald Transcript
dduvall edited edge metadata.Jul 10 2017, 4:58 PM

The pattern you've implemented looks legit. I've commented on mostly nit things.

docker/compiler.go
90

This will blowup if there's an error in NewDockerInstruction as dockerInstruction will be nil, but I guess that's cool for now since there's no real error handling in Compile. :)

docker/instructions.go
32

From what I've seen so far in the Go sphere, it's idiomatic to drop the Get from getters.

https://golang.org/doc/effective_go.html#Getters

Here I think it makes more sense to simply drop the getter altogether since Arguments is an exported field.

43

Whitespace nit.

51

Whitespace nit.

60

Whitespace nit.

dduvall requested changes to this revision.Jul 10 2017, 4:58 PM
This revision now requires changes to proceed.Jul 10 2017, 4:58 PM
thcipriani updated this revision to Diff 1852.Jul 10 2017, 5:38 PM
thcipriani edited edge metadata.

Idiomatic getter

dduvall accepted this revision.Jul 10 2017, 7:52 PM

Changes look good and you already mentioned that gofmt enforces that single-line struct format and upstream confirms that's just the way it is.

This revision is now accepted and ready to land.Jul 10 2017, 7:52 PM
This revision was automatically updated to reflect the committed changes.