act

act copied to clipboard

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 https://github.com/nektos/act/blob/7a426a0f37d21e32a01e85d9403edba17a54d545/cmd/root.go#L152-L153 https://github.com/nektos/act/blob/7a426a0f37d21e32a01e85d9403edba17a54d545/pkg/common/git.go#L276-L279 https://github.com/nektos/act/blob/7a426a0f37d21e32a01e85d9403edba17a54d545/pkg/container/docker_run.go#L549-L550 https://github.com/nektos/act/blob/7a426a0f37d21e32a01e85d9403edba17a54d545/pkg/model/planner.go#L92-L94 https://github.com/nektos/act/blob/7a426a0f37d21e32a01e85d9403edba17a54d545/pkg/model/workflow.go#L214-L217 https://github.com/nektos/act/blob/7a426a0f37d21e32a01e85d9403edba17a54d545/pkg/runner/expression.go#L131-L142 https://github.com/nektos/act/blob/7a426a0f37d21e32a01e85d9403edba17a54d545/pkg/runner/step_context.go#L454-L455

---

• [x] ~~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?

  estdata/
  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) https://github.com/nektos/act/blob/7a426a0f37d21e32a01e85d9403edba17a54d545/pkg/container/docker_logger.go#L25-L76 https://github.com/nektos/act/blob/7a426a0f37d21e32a01e85d9403edba17a54d545/pkg/runner/runner_test.go#L123

@cplee, i appreciate any thoughts you have on this

@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

• 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/ https://github.com/nektos/act/blob/ec34eb9532d314ba8d3c23472acf4037f2c084cd/pkg/runner/runner_test.go#L124
• we have unused tests, question is if it's useful to keep that since it was mostly used in GitHub Actions https://github.com/nektos/act/blob/ec34eb9532d314ba8d3c23472acf4037f2c084cd/pkg/runner/expression_test.go#L211 https://github.com/nektos/act/blob/ec34eb9532d314ba8d3c23472acf4037f2c084cd/pkg/runner/run_context_test.go#L162

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.