Code clean-up

[catthehacker]

I'm not the best person to describe things so pardon mess below

---

• revisit code with nolint
  amount of code with disabled linting keeps increasing, it should be checked if we can prevent that
  

  act/cmd/root.go

  Lines 152 to 153 in 7a426a0

  	//nolint:gocyclo
  	func newRunCommand(ctx context.Context, input *Input) func(*cobra.Command, []string) error {

  
  

  act/pkg/common/git.go

  Lines 276 to 279 in 7a426a0

  	// NewGitCloneExecutor creates an executor to clone git repos
  	// nolint:gocyclo
  	func NewGitCloneExecutor(input NewGitCloneExecutorInput) Executor {
  	return func(ctx context.Context) error {

  
  

  act/pkg/container/docker_run.go

  Lines 549 to 550 in 7a426a0

  	// nolint: gocyclo
  	func (cr *containerReference) copyDir(dstPath string, srcPath string, useGitIgnore bool) common.Executor {

  
  

  act/pkg/model/planner.go

  Lines 92 to 94 in 7a426a0

  	// NewWorkflowPlanner will load a specific workflow, all workflows from a directory or all workflows from a directory and its subdirectories
  	// nolint: gocyclo
  	func NewWorkflowPlanner(path string, noWorkflowRecurse bool) (WorkflowPlanner, error) {

  
  

  act/pkg/model/workflow.go

  Lines 214 to 217 in 7a426a0

  	// GetMatrixes returns the matrix cross product
  	// It skips includes and hard fails excludes for non-existing keys
  	// nolint:gocyclo
  	func (j *Job) GetMatrixes() []map[string]interface{} {

  
  

  act/pkg/runner/expression.go

  Lines 131 to 142 in 7a426a0

  	//nolint
  	switch {
  	default:
  	buf.WriteRune(c)
  	case c == '\'':
  	buf.WriteRune(c)
  	ee.advString(&buf, r)
  	case c == '.':
  	buf.WriteString("['")
  	ee.advPropertyName(&buf, r)
  	buf.WriteString("']")
  	}

  
  

  act/pkg/runner/step_context.go

  Lines 454 to 455 in 7a426a0

  	// nolint: gocyclo
  	func (sc *StepContext) runAction(actionDir string, actionPath string, localAction bool) common.Executor {

---

• add missing error checking where possible, where definitely sure it's not required/helpful, annotate it with a comment

---

• move all test data to single location and cross-test as much as we can
  testdata shouldn't be duplicated and cross-testing should help prevent issue
  it's fine if we have dedicated tests for some functions but most of it could be verified by all packages
• clean-up tests
  something like this?

   testdata/
   	events/
   		push/
   		pull_request/
   		...
   	issues/
   		issue-xxx/
   		issue-xxx/
   		...
   	uses/
   		uses-composite/
   		uses-docker-url/
   		...
   	shells/
   		sh/
   		...
   	...
   	*ungrouped tests*
  

---

• refactor logging
  currently logging in whole act is a mess (IMO), debug/info messages have been enabled/disabled/moved, error checking is missing in places, terminal is clogged with git and other spam
  my idea is to re-work all logging into central package (common/logger) and implement debug levels (e.g.: 1 - minimal debug info, 2 - additional debugging, 3 - all debug), it should be fine for local development to run with level 1/2, in some extreme cases one can enable 3 (which would be also default for CI)

---

• dead / unnecessary code (leftovers)
  

  act/pkg/container/docker_logger.go

  Lines 25 to 76 in 7a426a0

  	/*
  	func logDockerOutput(ctx context.Context, dockerResponse io.Reader) {
  	logger := common.Logger(ctx)
  	if entry, ok := logger.(*logrus.Entry); ok {
  	w := entry.Writer()
  	_, err := stdcopy.StdCopy(w, w, dockerResponse)
  	if err != nil {
  	logrus.Error(err)
  	}
  	} else if lgr, ok := logger.(*logrus.Logger); ok {
  	w := lgr.Writer()
  	_, err := stdcopy.StdCopy(w, w, dockerResponse)
  	if err != nil {
  	logrus.Error(err)
  	}
  	} else {
  	logrus.Errorf("Unable to get writer from logger (type=%T)", logger)
  	}
  	}
  	*/
  	/*
  	func streamDockerOutput(ctx context.Context, dockerResponse io.Reader) {
  	/*
  	out := os.Stdout
  	go func() {
  	<-ctx.Done()
  	//fmt.Println()
  	}()
  	_, err := io.Copy(out, dockerResponse)
  	if err != nil {
  	logrus.Error(err)
  	}
  	* /
  	logger := common.Logger(ctx)
  	reader := bufio.NewReader(dockerResponse)
  	for {
  	if ctx.Err() != nil {
  	break
  	}
  	line, _, err := reader.ReadLine()
  	if err == io.EOF {
  	break
  	}
  	logger.Debugf("%s\n", line)
  	}
  	}
  	*/

  
  

  act/pkg/runner/runner_test.go

  Line 123 in 7a426a0

  // {"testdata", "issue-228", "push", "", platforms, ""}, // TODO [igni]: Remove this once everything passes

[catthehacker]

@cplee, i appreciate any thoughts you have on this

[cplee]

@catthehacker thanks for taking initiative with this. Here's some thoughts:

• revisit nolint - this would be great to pull out as its own issue. This will be a bit challenging and requiring good test coverage first for each to ensure reducing cyclomatic complexity doesnt introduce bugs.
• missing error checks - agreed...do you have an example in mind? This would be a great first issue if folks are interested in helping
• consolidate test data - 💯
• cleanup tests - can you talk more about what the problem is that this is trying to fix?
• refactor logging - 💯
• dead code - for sure, rip it out! This would be a great first issue if folks are interested in helping

[catthehacker]

• missing error checks - agreed...do you have an example in mind? This would be a great first issue if folks are interested in helping

That probably was me going off from bad memory since I already cleaned that up in #679 😄

• cleanup tests - can you talk more about what the problem is that this is trying to fix?

Few points I noticed over time:

• Move out arm64 test to own func and remove containerArchitecture from TestJobFileInfo, I think it wasn't best decision to do it that way, also add docs about requirements for tests or install qemu arm64 automatically
  wip:

func TestRunDifferentArch(t *testing.T) {
	if testing.Short() {
		t.Skip("skipping integration test")
	}

	ctx := context.Background()
	client, err := container.GetDockerClient(ctx)
	assert.NoError(t, err)

	err = container.NewDockerPullExecutor(container.NewDockerPullExecutorInput{
		Image: "tonistiigi/binfmt:latest",
	})(ctx)
	assert.NoError(t, err)

	var platSpecs *specs.Platform
	_, err = client.ContainerCreate(ctx, &typesContainer.Config{
		Cmd:   []string{"--install", "linux/arm64"},
		Image: "tonistiigi/binfmt:latest",
	}, &typesContainer.HostConfig{
		AutoRemove: true,
		Privileged: true,
	}, &network.NetworkingConfig{}, platSpecs, "binfmt")
	assert.NoError(t, err)

	err = client.ContainerStart(ctx, "binfmt", types.ContainerStartOptions{})
	assert.NoError(t, err)

	platforms := map[string]string{
		"ubuntu-latest": baseImage,
	}

	log.SetLevel(log.DebugLevel)
	runTestJobFile(ctx, t, TestJobFileInfo{"testdata", "basic", "push", "", platforms, "linux/arm64"})
}

• we have unused testdata that either should be fixed/used or removed if it's not applicable or tested different way, below one test is disabled but we have more in testdata/
  

  act/pkg/runner/runner_test.go

  Line 124 in ec34eb9

  // {"testdata", "issue-228", "push", "", platforms, ""}, // TODO [igni]: Remove this once everything passes

• we have unused tests, question is if it's useful to keep that since it was mostly used in GitHub Actions
  

  act/pkg/runner/expression_test.go

  Line 211 in ec34eb9

  func updateTestExpressionWorkflow(t *testing.T, tables []struct {

  
  

  act/pkg/runner/run_context_test.go

  Line 162 in ec34eb9

  func updateTestIfWorkflow(t *testing.T, tables []struct {

[ChristopherHX]

We seem to have multiple nolint formatting in our codebase. I'm not shure, which is our prefered formatting of them.

• //nolint:<rule
• //nolint: <rule
• // nolint:<rule
• // nolint: <rule
  They only differ in spacing and the golang-cilint seem to accept all, I found all combinations with github search "nolint".

The docu of golang-cilint seem to use the first option in examples.