Update cloudbuild configuration making use of added deploy support for pulling images from GAR
#65
Conversation
Pull Request Test Coverage Report for Build 1343992846
💛 - Coveralls |
cmd/deploy/exec.go
Outdated
| c := exec.Command(cmd, args...) | ||
| c.Stdout = e.stdout | ||
| c.Stderr = e.stderr | ||
| log.Info(c.String()) |
There was a problem hiding this comment.
do we really want to log here?
There was a problem hiding this comment.
This is the current behavior, but I agree it is unnecessary and I have removed it.
cmd/deploy/specs.go
Outdated
| @@ -59,7 +75,8 @@ type KindSpec struct { | |||
| Wait time.Duration `yaml:"wait"` | |||
| Kubecfg string `yaml:"kubecfg"` | |||
| DeployWithClient bool `yaml:"deployWithClient"` | |||
| execer func(string, ...string) error | |||
| GoogleArtifactRegistries []string `yaml:"googleArtifactRegistries"` | |||
| execer execerInterface | |||
There was a problem hiding this comment.
why attach this here?
i would prefer to keep the config "pure"
if we want to have an "execer" then we should wrap the spec with another struct wiith that attached
There was a problem hiding this comment.
I would argue this is no less "pure" than before, adding the extra layer is an option that I can implement. If there is no reason for an execer to belong to each spec (and if each spec may need access to it) then what about a global variable:
var (
execer execerInterface = exec.NewExecer()
)
Specs in the file can directly call execer.Exec() and tests can stub execer to equal exec.NewFakeExecer()
That would work in my head but not sure which is better, any thoughts?
cmd/deploy/exec.go
Outdated
| @@ -0,0 +1,64 @@ | |||
| package deploy | |||
There was a problem hiding this comment.
this feels like there should be an implementation here in a seperate package
There was a problem hiding this comment.
I moved this code into its own package (exec) that can be used here or elsewhere, there is nothing inherent to deployment. Where do you think the best place for this file is? Maybe a new top level directory for utilities?
Not sure what you mean by implementation. My idea was to have this library provide the implementation of an execer/fake only, and the user of the library to define an interface (like in specs.go). This is following the internal google style guide, are you suggesting something different?
There was a problem hiding this comment.
i was saying the have a package just for exec
and then do exactly what you are saying - users would just use an interface to make it testable
I don't like "collection" directories
kne/os/execer seems fine path?
| return nil | ||
| } | ||
|
|
||
| func writeDockerConfig(path string, registries []string) error { |
There was a problem hiding this comment.
would a template be nicer to use here?
| @@ -0,0 +1,72 @@ | |||
| apiVersion: apiextensions.k8s.io/v1 | |||
cmd/deploy/exec.go
Outdated
| @@ -0,0 +1,61 @@ | |||
| package exec | |||
There was a problem hiding this comment.
go build will not like this
| return err | ||
| } | ||
| defer os.RemoveAll(tempDockerDir) | ||
| originalConfig := os.Getenv(dockerConfigEnvVar) |
There was a problem hiding this comment.
i would maybe suggest a docstring here on why and what you are doing here - it probably isn't super obvious why this is necessary
Changes:
TODO in future change: