apply: to check module requirements before run #358
Conversation
modules must now provide a `make check` in their root makefile make check must exit 0 on all modules before proceeding to apply this allows checks for module specific binaries or check tokens permissions
|
Mind pasting text or posting a screenshot of what the final output looks like after this change? |
|
|
|
Hmm I wonder if we should be checking for return code 2 (a problem with make itself, like specifying a target that doesn't exist) and provide a slightly different message to indicate that it's likely a problem with the module itself. |
check:
exit 1Looks like even when i have the make target exits with error code 1, the exec still exits with code 2, i tried to add a binary check as follow, the code returned from exec.cmd is still REQUIRED_BINS := gh foobar
check:
$(foreach bin, $(REQUIRED_BINS),\
$(if $(shell command -v $(bin) 2> /dev/null),$(info Found `$(bin)`),$(error Please install `$(bin)`)))but it can still show the error if the module is diligent with printing the error since we pipe it out |
|
Hmm okay. In that case we could check stderr for the string "No rule to make target" |
internal/apply/apply.go
Outdated
| flog.Infof("Infrastructure executor: %s", "Terraform") | ||
|
|
||
| applyAll(rootDir, *projectConfig, environments) | ||
| err = modulesWalkCmd("apply", rootDir, projectConfig, []string{"make"}, environments) |
There was a problem hiding this comment.
Will this stop on the first one that fails? We should really test all the modules, and then return the state / output of all the tests. It wouldn't be a great experience to have to run the app multiple times, installing stuff between runs.
There was a problem hiding this comment.
yeah good point, right now it fails on the first error
|
so the new behavior looks like this, if the check uses a script it wont have the makefile noise in the output
|
|
Cool! Can we suppress the output as we are executing the command so we can just present Zero's nicer output? I guess we would just have to optionally disable the multi writer in |
internal/apply/apply_test.go
Outdated
|
|
||
| }) | ||
|
|
||
| t.Run("Moudles runs command overides", func(t *testing.T) { |
There was a problem hiding this comment.
This and below say "Moudles"
There was a problem hiding this comment.
That does have a nice ring to it though..
internal/util/util.go
Outdated
| }() | ||
| go func() { | ||
| _, errStderr = io.Copy(os.Stderr, stderrPipe) | ||
| strErrStreams := []io.Writer{errContent} |
There was a problem hiding this comment.
is this supposed to be stdErrStreams?
There was a problem hiding this comment.
sorry yeah, should have paid more attention with all these typos
modules must now provide a
make checkin their root makefilemake check must exit 0 on all modules before proceeding to apply
this allows checks for module specific binaries or check tokens permissions