starred/act

Fork 0

mirror of https://github.com/nektos/act.git synced 2026-04-26 01:15:51 +03:00

[GH-ISSUE #831] Code clean-up #506

New issue

Open

opened 2026-03-01 21:44:01 +03:00 by kerem · 4 comments

kerem commented

2026-03-01 21:44:01 +03:00

Owner

Originally created by @catthehacker on GitHub (Sep 27, 2021).
Original GitHub issue: https://github.com/nektos/act/issues/831

Originally assigned to: @catthehacker on GitHub.

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

~~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)
github.com/nektos/act@7a426a0f37/pkg/container/docker_logger.go (L25-L76)
github.com/nektos/act@7a426a0f37/pkg/runner/runner_test.go (L123)

Originally created by @catthehacker on GitHub (Sep 27, 2021). Original GitHub issue: https://github.com/nektos/act/issues/831 Originally assigned to: @catthehacker on GitHub. 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? ``` 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) 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

kerem added the

needs-work

stale-exempt

kind/discussion

labels

2026-03-01 21:44:01 +03:00

kerem commented

2026-03-01 21:44:02 +03:00

Author

Owner

@catthehacker commented on GitHub (Sep 27, 2021):

@cplee, i appreciate any thoughts you have on this

@catthehacker commented on GitHub (Sep 27, 2021): @cplee, i appreciate any thoughts you have on this

kerem commented

2026-03-01 21:44:02 +03:00

Author

Owner

@cplee commented on GitHub (Oct 18, 2021):

@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

@cplee commented on GitHub (Oct 18, 2021): @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

kerem commented

2026-03-01 21:44:02 +03:00

Author

Owner

@catthehacker commented on GitHub (Nov 13, 2021):

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/
github.com/nektos/act@ec34eb9532/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
github.com/nektos/act@ec34eb9532/pkg/runner/expression_test.go (L211)
github.com/nektos/act@ec34eb9532/pkg/runner/run_context_test.go (L162)

@catthehacker commented on GitHub (Nov 13, 2021): > * 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: ```go 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

kerem commented

2026-03-01 21:44:02 +03:00

Author

Owner

@ChristopherHX commented on GitHub (Sep 17, 2022):

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.

@ChristopherHX commented on GitHub (Sep 17, 2022): 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.

kerem referenced this issue

2026-03-01 21:51:23 +03:00

[PR #530] [MERGED] Fixed reference not found error when using commit hash. #1487

No milestone

No project

No assignees

1 participant

Notifications

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

starred/act#506

No description provided.

Rows
Columns