Avoid using `log.Fatal` in `pkg/*` #1705

wolfogre · 2023-03-28T07:52:56Z

Although act is a command line tool, it is also a bad idea to use log.Fatal in packages.

To be clear, only packages which are used as libraries. It's fine to use fatal in cmd.

This reverts commit 6599803.

mergify · 2023-03-28T07:55:58Z

@wolfogre this pull request has failed checks 🛠

wolfogre · 2023-03-28T07:54:26Z

pkg/common/outbound_ip.go

+		// return nil instead of fatal
+		return nil


Just like what net.ParseIP does.

I think it might make sense to return the err here instead of just nil.
Return only nil will hide the error that could be numerous things as net.Dial does connect to a server. I know it's UDP and therefore stateless (no packet is transferred here).
If you are sure that it could only be an IP parsing error, I'm fine with returning nil.

In fact, I noticed that it cannot work without internet access, so #1707

wolfogre · 2023-03-28T07:56:02Z

pkg/model/workflow.go

@@ -670,3 +664,17 @@ func (w *Workflow) GetJobIDs() []string {
 	}
 	return ids
 }
+
+var OnDecodeNodeError = func(node yaml.Node, out interface{}, err error) {


So it's possible to ignore errors when decoding nodes by setting OnDecodeNodeError to nil.

I understand changing all methods to return errors is a much larger task than adding a static handler, better than overriding exit in global logrus object...

Other option would be panic with a custom type and catch it by the Planner to return an error like the official json lib.

KnisterPeter · 2023-03-28T07:59:48Z

Hi @wolfogre, can you explain a bit on the reasoning of your changes. Are you using some packages outside of act? Did you stumble upon an error?

ChristopherHX · 2023-03-28T08:07:08Z

If you use act in a self-hosted runner (see https://nektosact.com/integrations.html) you don't want the runner to terminate on such an error. Instead only the job request should fail.

I know @KnisterPeter uses the act cli and not the golang packages, so you are not affected

wolfogre · 2023-03-28T08:07:09Z

Are you using some packages outside of act?

TBH, yes. I am a maintainer of Gitea. And Gitea has a CICD runner named act_runner, it based on a soft fork of act.

More context:

mergify · 2023-03-28T08:16:59Z

@wolfogre this pull request has failed checks 🛠

wolfogre · 2023-03-28T08:25:03Z

this pull request has failed checks 🛠

Can the maintainers help take a look?

https://github.com/nektos/act/actions/runs/4540659358/jobs/8001848574?pr=1705

KnisterPeter · 2023-03-28T08:27:16Z

@wolfogre Ah I see. The missing piece for me was that you are a maintainer of gitea. That makes sense.

codecov · 2023-03-28T08:42:51Z

Codecov Report

Merging #1705 (9ffc5a6) into master (4989f44) will increase coverage by 1.22%.
The diff coverage is 67.03%.

@@            Coverage Diff             @@
##           master    #1705      +/-   ##
==========================================
+ Coverage   61.22%   62.44%   +1.22%     
==========================================
  Files          46       48       +2     
  Lines        7141     7533     +392     
==========================================
+ Hits         4372     4704     +332     
- Misses       2462     2505      +43     
- Partials      307      324      +17

Impacted Files	Coverage Δ
pkg/common/outbound_ip.go	`0.00% <0.00%> (ø)`
pkg/container/docker_cli.go	`82.23% <ø> (ø)`
pkg/container/docker_logger.go	`52.08% <ø> (ø)`
pkg/container/docker_pull.go	`33.33% <ø> (ø)`
pkg/container/docker_run.go	`14.08% <0.00%> (+0.49%)`	⬆️
pkg/container/docker_volume.go	`0.00% <ø> (ø)`
pkg/container/file_collector.go	`39.68% <0.00%> (+2.38%)`	⬆️
pkg/container/host_environment.go	`0.00% <0.00%> (ø)`
pkg/exprparser/functions.go	`66.32% <0.00%> (-1.04%)`	⬇️
pkg/model/workflow.go	`41.68% <13.33%> (-0.44%)`	⬇️
... and 21 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

mergify · 2023-03-31T13:10:00Z

@wolfogre this pull request is now in conflict 😩

mergify · 2023-03-31T14:21:47Z

@wolfogre this pull request has failed checks 🛠

ChristopherHX · 2023-03-31T14:56:55Z

Seems like gitea and act_runner could also tell logrus to do anything else, I don't really know where global logrus is used in gitea and if log.Fatal() should continue to terminate.

// You can edit this code!
// Click here and start typing.
package main

import (
	"fmt"

	"github.com/sirupsen/logrus"
)

func main() {

	logrus.StandardLogger().ExitFunc = func(s int) {
		fmt.Println("Hello, 世界")
	}
	logrus.StandardLogger().Fatal("MEEE")
	logrus.Fatal("MEEE")
	fmt.Println("Hello, 世界")
}

https://go.dev/play/p/TFKi-2hf4fu

ChristopherHX

Looks good to me.

ChristopherHX · 2023-03-31T15:47:36Z

pkg/model/workflow.go

@@ -670,3 +664,17 @@ func (w *Workflow) GetJobIDs() []string {
 	}
 	return ids
 }
+
+var OnDecodeNodeError = func(node yaml.Node, out interface{}, err error) {


I understand changing all methods to return errors is a much larger task than adding a static handler, better than overriding exit in global logrus object...

Other option would be panic with a custom type and catch it by the Planner to return an error like the official json lib.

mergify · 2023-04-17T08:56:33Z

@wolfogre this pull request has failed checks 🛠

mergify · 2023-04-17T09:44:13Z

@wolfogre this pull request has failed checks 🛠

Follow nektos#1705 Reviewed-on: https://gitea.com/gitea/act/pulls/39 Reviewed-by: Lunny Xiao <xiaolunwen@gmail.com> Co-authored-by: Jason Song <i@wolfogre.com> Co-committed-by: Jason Song <i@wolfogre.com>

wolfogre added 6 commits March 28, 2023 15:39

fix: common

6599803

fix: in runner

0fb5327

fix: decodeNode

02ae778

fix: GetMatrixes

6a2a1b7

Revert "fix: common"

9ab71eb

This reverts commit 6599803.

fix: GetOutboundIP

fc50955

wolfogre requested a review from a team as a code owner March 28, 2023 07:52

pull-request-size bot added the size/M label Mar 28, 2023

mergify bot added the needs-work Extra attention is needed label Mar 28, 2023

wolfogre commented Mar 28, 2023

View reviewed changes

test: fix cases

7b92a8e

mergify bot removed the needs-work Extra attention is needed label Mar 28, 2023

wolfogre mentioned this pull request Mar 28, 2023

actions: crash if action yaml cannot be parsed go-gitea/gitea#23658

Closed

mergify bot added the conflict PR has conflicts label Mar 31, 2023

Merge branch 'master' into bugfix/avoid_log_fatal

a2413a0

mergify bot removed the conflict PR has conflicts label Mar 31, 2023

mergify bot added the needs-work Extra attention is needed label Mar 31, 2023

mergify bot removed the needs-work Extra attention is needed label Mar 31, 2023

ChristopherHX approved these changes Mar 31, 2023

View reviewed changes

ae-ou mentioned this pull request Apr 2, 2023

feature: reference workflowpattern package to add workflow filtering #1709

Closed

wolfogre requested a review from KnisterPeter April 3, 2023 03:27

Merge branch 'master' into bugfix/avoid_log_fatal

cc20176

mergify bot added the needs-work Extra attention is needed label Apr 17, 2023

mergify bot removed the needs-work Extra attention is needed label Apr 17, 2023

KnisterPeter approved these changes Apr 18, 2023

View reviewed changes

cplee approved these changes Apr 18, 2023

View reviewed changes

Merge branch 'master' into bugfix/avoid_log_fatal

9ffc5a6

mergify bot merged commit 816a7d4 into nektos:master Apr 18, 2023

Zettat123 mentioned this pull request Apr 24, 2023

Avoid using log.Fatal in pkg/* #1759

Merged

wolfogre mentioned this pull request Dec 13, 2023

WorkflowDispatchConfig supports multiple yaml node kinds #2123

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Avoid using `log.Fatal` in `pkg/*` #1705

Avoid using `log.Fatal` in `pkg/*` #1705

wolfogre commented Mar 28, 2023

mergify bot commented Mar 28, 2023

wolfogre Mar 28, 2023

KnisterPeter Mar 28, 2023

wolfogre Mar 28, 2023

wolfogre Mar 28, 2023

ChristopherHX Mar 31, 2023

KnisterPeter commented Mar 28, 2023

ChristopherHX commented Mar 28, 2023

wolfogre commented Mar 28, 2023

mergify bot commented Mar 28, 2023

wolfogre commented Mar 28, 2023 •

edited

Loading

KnisterPeter commented Mar 28, 2023

codecov bot commented Mar 28, 2023 •

edited

Loading

mergify bot commented Mar 31, 2023

mergify bot commented Mar 31, 2023

ChristopherHX commented Mar 31, 2023

ChristopherHX left a comment

ChristopherHX Mar 31, 2023

mergify bot commented Apr 17, 2023

mergify bot commented Apr 17, 2023

Avoid using log.Fatal in pkg/* #1705

Avoid using log.Fatal in pkg/* #1705

Conversation

wolfogre commented Mar 28, 2023

mergify bot commented Mar 28, 2023

wolfogre Mar 28, 2023

Choose a reason for hiding this comment

KnisterPeter Mar 28, 2023

Choose a reason for hiding this comment

wolfogre Mar 28, 2023

Choose a reason for hiding this comment

wolfogre Mar 28, 2023

Choose a reason for hiding this comment

ChristopherHX Mar 31, 2023

Choose a reason for hiding this comment

KnisterPeter commented Mar 28, 2023

ChristopherHX commented Mar 28, 2023

wolfogre commented Mar 28, 2023

mergify bot commented Mar 28, 2023

wolfogre commented Mar 28, 2023 • edited Loading

KnisterPeter commented Mar 28, 2023

codecov bot commented Mar 28, 2023 • edited Loading

Codecov Report

mergify bot commented Mar 31, 2023

mergify bot commented Mar 31, 2023

ChristopherHX commented Mar 31, 2023

ChristopherHX left a comment

Choose a reason for hiding this comment

ChristopherHX Mar 31, 2023

Choose a reason for hiding this comment

mergify bot commented Apr 17, 2023

mergify bot commented Apr 17, 2023

Avoid using `log.Fatal` in `pkg/*` #1705

Avoid using `log.Fatal` in `pkg/*` #1705

wolfogre commented Mar 28, 2023 •

edited

Loading

codecov bot commented Mar 28, 2023 •

edited

Loading