Suppressing `423` error in Terragrunt Provider Cache #3453

levkohimins · 2024-10-03T23:22:36Z

Description

Fixes #3430.
Fixes #3420.

TODOs

Read the Gruntwork contribution guidelines.

Update the docs.
Run the relevant tests successfully, including pre-commit checks.
Ensure any 3rd party code adheres with our license policy or delete this line if its not applicable.
Include release notes. If this PR is backward incompatible, include a migration guide.

Release Notes (draft)

Added / Removed / Updated [X].

Migration Guide

denis256 · 2024-10-04T14:18:05Z

internal/errors/errors.go

+
+// WithPanicHandling wraps every command you add to *cli.App to handle panics by logging them with a stack trace and returning
+// an error up the chain.
+func WithPanicHandling(action func(c *cli.Context) error) func(c *cli.Context) error {


WithPanicHandling seems to be unused, is it intended?

$ grep -irn "WithPanicHandling" . ./internal/errors/errors.go:106:// WithPanicHandling wraps every command you add to *cli.App to handle panics by logging them with a stack trace and returning ./internal/errors/errors.go:108:func WithPanicHandling(action func(c *cli.Context) error) func(c *cli.Context) error

Indeed. Removed.

denis256 · 2024-10-04T14:30:08Z

internal/errors/errors.go

+}
+
+// ErrorWithExitCode is a custom error that is used to specify the app exit code.
+type ErrorWithExitCode struct {


Should the name conform to the XxxError format?
Like ExitCodeError

denis256 · 2024-10-04T14:40:42Z

test/integration_aws_test.go

@@ -951,11 +951,11 @@ func TestAwsMockOutputsFromRemoteState(t *testing.T) { //nolint: paralleltest
 require.NoError(t, os.Remove(filepath.Join(environmentPath, "/app1/.terraform/terraform.tfstate")))
 require.NoError(t, os.RemoveAll(filepath.Join(environmentPath, "/app1/.terraform")))

- _, stderr, err := runTerragruntCommandWithOutput(t, fmt.Sprintf("terragrunt init --terragrunt-fetch-dependency-output-from-state --terragrunt-non-interactive --terragrunt-working-dir %s/app2", environmentPath))


Seems to fail test TestAwsMockOutputsFromRemoteState after multiple CI reruns

denis256 · 2024-10-07T13:59:15Z

I noticed strict linter complaining about some moved code, most probably we need another issue to address the findings

internal/os/exec/ptty_unix.go:24: Function 'runCommandWithPTY' is too long (75 > 60) (funlen)
func runCommandWithPTY(logger log.Logger, cmd *exec.Cmd) (err error) {
config/include.go:102: the line is 228 characters long, which exceeds the maximum of 140 characters. (lll)
                return nil, errors.New("you reached an impossible condition. This is most likely a bug in terragrunt. Please open an issue at github.com/gruntwork-io/terragrunt with this error message.Code: HANDLE_INCLUDE_NIL_INCLUDE_CONFIG")
config/include.go:168: the line is 240 characters long, which exceeds the maximum of 140 characters. (lll)
                return nil, errors.New("you reached an impossible condition. This is most likely a bug in terragrunt. Please open an issue at github.com/gruntwork-io/terragrunt with this error message. Code: HANDLE_INCLUDE_DEPENDENCY_NIL_INCLUDE_CONFIG")
cli/commands/render-json/action.go:35: the line is 269 characters long, which exceeds the maximum of 140 characters. (lll)
                return errors.New("terragrunt was not able to render the config as json because it received no config. This is almost certainly a bug in Terragrunt. Please open an issue on github.com/gruntwork-io/terragrunt with this message and the contents of your terragrunt.hcl")
internal/os/signal/signal_unix.go:15:22: var-declaration: should omit type []os.Signal from declaration of var InterruptSignals; it will be inferred from the right-hand side (revive)
var InterruptSignals []os.Signal = []os.Signal{syscall.SIGTERM, syscall.SIGINT}
                     ^
internal/os/exec/ptty_unix.go:102:1: exported: exported function PrepareConsole should have comment or be unexported (revive)
func PrepareConsole(logger log.Logger) {
^
internal/errors/errors.go:13:9: do not define dynamic errors, use wrapped static errors instead: "fmt.Errorf(message, args...)" (err113)
        err := fmt.Errorf(message, args...)
               ^
internal/errors/errors.go:99:10: do not define dynamic errors, use wrapped static errors instead: "fmt.Errorf(\"%v\", rec)" (err113)
                        err = fmt.Errorf("%v", rec)
                              ^
internal/errors/export.go:18:9: do not define dynamic errors, use wrapped static errors instead: "errors.New(text)" (err113)
        return errors.New(text)
               ^
internal/os/signal/signal_unix.go:12:5: InterruptSignal is a global variable (gochecknoglobals)
var InterruptSignal = syscall.SIGINT

denis256 · 2024-10-07T14:05:06Z

Something is not right with error handling I suspect

terragrunt version v0.67.16

14:02:51.934 ERROR  5 errors occurred:
	* [./app2] command failed with exit code 1
	* [./app3] command failed with exit code 1
	* [./app4] command failed with exit code 1
	* [./app5] command failed with exit code 1
	* [./app1] command failed with exit code 1


14:02:51.934 ERROR  Unable to determine underlying exit code, so Terragrunt will exit with error code 1
14:02:51.935 ERROR  Suggested fixes: 
The executables 'terraform' and 'tofu' are missing from your $PATH. Please add at least one of these to your $PATH.

terragrunt version v0.67.16-18-g55a5d7bea90e

14:04:48.674 ERROR  5 errors occurred:
	* %!s(PANIC=Error method: runtime error: invalid memory address or nil pointer dereference)
	* %!s(PANIC=Error method: runtime error: invalid memory address or nil pointer dereference)
	* %!s(PANIC=Error method: runtime error: invalid memory address or nil pointer dereference)
	* %!s(PANIC=Error method: runtime error: invalid memory address or nil pointer dereference)
	* %!s(PANIC=Error method: runtime error: invalid memory address or nil pointer dereference)


14:04:48.677 ERROR  Unable to determine underlying exit code, so Terragrunt will exit with error code 1
14:04:48.677 ERROR  runtime error: invalid memory address or nil pointer dereference
14:04:48.678 ERROR  Unable to determine underlying exit code, so Terragrunt will exit with error code 1

yhakbar

Nothing major in my review. I'll avoid submitting approval, as it seems Denis has found some significant issues with error handling.

yhakbar · 2024-10-07T13:24:00Z

configstack/running_module.go

@@ -165,7 +165,7 @@ func (module *RunningModule) moduleFinished(moduleErr error) {
 if moduleErr == nil {
 module.Module.TerragruntOptions.Logger.Debugf("Module %s has finished successfully!", module.Module.Path)
 } else {
- module.Module.TerragruntOptions.Logger.Errorf("Module %s has finished with an error: %v", module.Module.Path, moduleErr)
+ module.Module.TerragruntOptions.Logger.Errorf("Module %s has finished with an error", module.Module.Path)


Why aren't we wrapping moduleErr here?

We display this error in the last steps. Since this error contains multiple lines, it is better to avoid duplicating it in the log.

yhakbar · 2024-10-07T13:27:30Z

configstack/test_helpers_test.go

+ errors.As(actualError, &multiError)
+ require.NotNil(t, multiError, "Expected a MutliError, but got: %v", actualError)
+
+ assert.Equal(t, len(expectedErrors), len(multiError.Errors))


Would assert.Len have been more useful here?

It definitely fits better. Thanks.

yhakbar · 2024-10-07T13:34:34Z

internal/errors/export.go

@@ -0,0 +1,31 @@
+package errors


Why is this better than using the standard library directly? It seems to me that we should just use the standard library running Is, As, etc.

This is just for convenience, in order not to have extra imports that require manual assignment of aliases.

import goerrors "errors"

yhakbar · 2024-10-07T13:36:55Z

internal/os/exec/cmd.go

+}
+
+// RegisterGracefullyShutdown registers a graceful shutdown for the command in two ways:
+// 1. If the context cacnel contains a cause with a signal, this means that Terragrunt received the signal from the OS,


Suggested change

// 1. If the context cacnel contains a cause with a signal, this means that Terragrunt received the signal from the OS,

// 1. If the context cancel contains a cause with a signal, this means that Terragrunt received the signal from the OS,

yhakbar · 2024-10-07T13:41:18Z

internal/os/exec/cmd.go

+ }, sig)
+
+ if cmd.forwardSignalDelay > 0 {
+ cmd.logger.Infof("%s signal will be forwarded to %s with delay %s",


Just double-checking here: Are you sure this should be in info?

yhakbar · 2024-10-07T13:41:54Z

internal/os/exec/cmd.go

+
+// SendSignal sends the given `sig` to the executed command.
+func (cmd *Cmd) SendSignal(sig os.Signal) {
+ cmd.logger.Infof("%s signal is forwarded to %s", cases.Title(language.English).String(sig.String()), cmd.filename)


Just double-checking here: Are you sure this should this be in info?

No, you are most likely right, changed to Debug

yhakbar · 2024-10-07T13:42:35Z

internal/os/exec/cmd.go

+ cmd.logger.Infof("%s signal is forwarded to %s", cases.Title(language.English).String(sig.String()), cmd.filename)
+
+ if err := cmd.Process.Signal(sig); err != nil {
+ cmd.logger.Errorf("Failed to forwarding signal %s to %s: %v", sig, cmd.filename, err)


Why isn't this error being wrapped via %w?

This is a log message, not returned error. I actually do not really know if there is a sense to wrap it when we display it as a text.

yhakbar · 2024-10-07T13:45:37Z

internal/os/exec/cmd_unix_test.go

+ runChannel <- cmd.Run()
+ }()
+
+ time.Sleep(1000 * time.Millisecond)


This might be a dumb question, but why aren't we waiting 1 * time.Second?

I didn't pay attention, and you're right, I fixed it.

yhakbar · 2024-10-07T13:46:39Z

internal/os/exec/cmd_unix_test.go

+ runChannel <- cmd.Run()
+ }()
+
+ time.Sleep(1000 * time.Millisecond)


Same potentially dumb question here.

I didn't pay attention, and you're right, I fixed it.

yhakbar · 2024-10-07T14:03:42Z

shell/run_shell_cmd.go

 if opts.JSONLogFormat && opts.TerraformLogsToJSON {
 logger := opts.Logger.WithField("workingDir", opts.WorkingDir).WithField("executedCommandArgs", args)
 outWriter = logger.WithOptions(log.WithOutput(errWriter)).Writer()
 errWriter = logger.WithOptions(log.WithOutput(errWriter)).WriterLevel(log.ErrorLevel)
- } else if command == opts.TerraformPath {


levkohimins · 2024-10-07T23:21:25Z

Something is not right with error handling I suspect

terragrunt version v0.67.16

14:02:51.934 ERROR  5 errors occurred:
	* [./app2] command failed with exit code 1
	* [./app3] command failed with exit code 1
	* [./app4] command failed with exit code 1
	* [./app5] command failed with exit code 1
	* [./app1] command failed with exit code 1


14:02:51.934 ERROR  Unable to determine underlying exit code, so Terragrunt will exit with error code 1
14:02:51.935 ERROR  Suggested fixes: 
The executables 'terraform' and 'tofu' are missing from your $PATH. Please add at least one of these to your $PATH.

terragrunt version v0.67.16-18-g55a5d7bea90e

14:04:48.674 ERROR  5 errors occurred:
	* %!s(PANIC=Error method: runtime error: invalid memory address or nil pointer dereference)
	* %!s(PANIC=Error method: runtime error: invalid memory address or nil pointer dereference)
	* %!s(PANIC=Error method: runtime error: invalid memory address or nil pointer dereference)
	* %!s(PANIC=Error method: runtime error: invalid memory address or nil pointer dereference)
	* %!s(PANIC=Error method: runtime error: invalid memory address or nil pointer dereference)


14:04:48.677 ERROR  Unable to determine underlying exit code, so Terragrunt will exit with error code 1
14:04:48.677 ERROR  runtime error: invalid memory address or nil pointer dereference
14:04:48.678 ERROR  Unable to determine underlying exit code, so Terragrunt will exit with error code 1

Fixed.

levkohimins · 2024-10-07T23:25:26Z

I noticed strict linter complaining about some moved code, most probably we need another issue to address the findings

internal/os/exec/ptty_unix.go:24: Function 'runCommandWithPTY' is too long (75 > 60) (funlen)
func runCommandWithPTY(logger log.Logger, cmd *exec.Cmd) (err error) {
config/include.go:102: the line is 228 characters long, which exceeds the maximum of 140 characters. (lll)
                return nil, errors.New("you reached an impossible condition. This is most likely a bug in terragrunt. Please open an issue at github.com/gruntwork-io/terragrunt with this error message.Code: HANDLE_INCLUDE_NIL_INCLUDE_CONFIG")
config/include.go:168: the line is 240 characters long, which exceeds the maximum of 140 characters. (lll)
                return nil, errors.New("you reached an impossible condition. This is most likely a bug in terragrunt. Please open an issue at github.com/gruntwork-io/terragrunt with this error message. Code: HANDLE_INCLUDE_DEPENDENCY_NIL_INCLUDE_CONFIG")
cli/commands/render-json/action.go:35: the line is 269 characters long, which exceeds the maximum of 140 characters. (lll)
                return errors.New("terragrunt was not able to render the config as json because it received no config. This is almost certainly a bug in Terragrunt. Please open an issue on github.com/gruntwork-io/terragrunt with this message and the contents of your terragrunt.hcl")
internal/os/signal/signal_unix.go:15:22: var-declaration: should omit type []os.Signal from declaration of var InterruptSignals; it will be inferred from the right-hand side (revive)
var InterruptSignals []os.Signal = []os.Signal{syscall.SIGTERM, syscall.SIGINT}
                     ^
internal/os/exec/ptty_unix.go:102:1: exported: exported function PrepareConsole should have comment or be unexported (revive)
func PrepareConsole(logger log.Logger) {
^
internal/errors/errors.go:13:9: do not define dynamic errors, use wrapped static errors instead: "fmt.Errorf(message, args...)" (err113)
        err := fmt.Errorf(message, args...)
               ^
internal/errors/errors.go:99:10: do not define dynamic errors, use wrapped static errors instead: "fmt.Errorf(\"%v\", rec)" (err113)
                        err = fmt.Errorf("%v", rec)
                              ^
internal/errors/export.go:18:9: do not define dynamic errors, use wrapped static errors instead: "errors.New(text)" (err113)
        return errors.New(text)
               ^
internal/os/signal/signal_unix.go:12:5: InterruptSignal is a global variable (gochecknoglobals)
var InterruptSignal = syscall.SIGINT

I fixed almost everything except the functions that are too long.

denis256

👍

denis256 · 2024-10-08T13:22:14Z

internal/errors/errors.go

+
+// Errorf creates a new error with the given format and values.
+// It can be used as a drop-in replacement for fmt.Errorf() to provide descriptive errors in return values.
+// If none of the given values contains an stack trace, it will be created.


"a stack trace" (in all places)

Ah, thanks, I will fix it in the next PR.

levkohimins added 14 commits September 23, 2024 21:57

fix: host discovery

5783928

fix: tests

2d65014

Merge branch 'main' into bug/provider-cache-suppress-423-error

1541546

chore: shell package code refactoring

09a11c7

chore: code comment

3d8937c

chore: fix tests

262f707

chore: fix tests

8692e8c

chore: fix tests

6960b95

chore: fix tests

9e42d41

chore: grammar

4b9ebac

fix: tests

50d52e6

chore: fix test

4c5e9de

chore: fix test

faf04db

Merge branch 'main' into bug/provider-cache-suppress-423-error

0fa8ed5

levkohimins changed the title ~~Fixing 423 error suppression in Terragrunt Provider Cache~~ Fixing 423 error in Terragrunt Provider Cache Oct 3, 2024

levkohimins changed the title ~~Fixing 423 error in Terragrunt Provider Cache~~ Suppressing 423 error in Terragrunt Provider Cache Oct 3, 2024

levkohimins marked this pull request as ready for review October 3, 2024 23:39

levkohimins requested review from denis256 and yhakbar as code owners October 3, 2024 23:39

levkohimins added 2 commits October 4, 2024 01:48

fix: signal package

37943c1

fix: lint

3ba6295

denis256 reviewed Oct 4, 2024

View reviewed changes

chore: fix tests

d1c515f

levkohimins requested a review from denis256 October 4, 2024 21:33

chore: fix strict lint

55a5d7b

yhakbar reviewed Oct 7, 2024

View reviewed changes

levkohimins added 2 commits October 7, 2024 21:07

chore: error displaying improvements

439617a

chore: fix tests

715d0cd

levkohimins added 6 commits October 7, 2024 21:26

chore: fix grammar

ab593a8

chore: fix tests

eba033c

chore: fix tests

b6f9da1

chore: fix tests

0f916ff

chore: fix strict lint

4ff851e

chore: code improvements

0f65b2f

chore: code improvements

02af63b

levkohimins requested a review from yhakbar October 8, 2024 07:28

chore: fix tests

52f529d

denis256 approved these changes Oct 8, 2024

View reviewed changes

levkohimins merged commit afd5d7f into main Oct 8, 2024
4 of 5 checks passed

levkohimins deleted the bug/provider-cache-suppress-423-error branch October 8, 2024 21:44

BrewTestBot mentioned this pull request Oct 9, 2024

terragrunt 0.68.0 Homebrew/homebrew-core#193411

Merged

kayiplar mentioned this pull request Oct 10, 2024

Terraform output not shown for run-all commands #3463

Closed

2 tasks

levkohimins mentioned this pull request Oct 10, 2024

Suppressing 423 error in Terragrunt Provider Cache #3466

Closed

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Suppressing `423` error in Terragrunt Provider Cache #3453

Suppressing `423` error in Terragrunt Provider Cache #3453

levkohimins commented Oct 3, 2024

denis256 Oct 4, 2024

levkohimins Oct 4, 2024

denis256 Oct 4, 2024

levkohimins Oct 4, 2024 •

edited

Loading

denis256 Oct 4, 2024

levkohimins Oct 4, 2024

denis256 commented Oct 7, 2024

denis256 commented Oct 7, 2024

yhakbar left a comment

yhakbar Oct 7, 2024

levkohimins Oct 8, 2024

yhakbar Oct 7, 2024

levkohimins Oct 8, 2024

yhakbar Oct 7, 2024

levkohimins Oct 8, 2024 •

edited

Loading

yhakbar Oct 7, 2024

levkohimins Oct 8, 2024

yhakbar Oct 7, 2024

yhakbar Oct 7, 2024

levkohimins Oct 8, 2024

yhakbar Oct 7, 2024

levkohimins Oct 8, 2024

yhakbar Oct 7, 2024

levkohimins Oct 8, 2024

yhakbar Oct 7, 2024

levkohimins Oct 8, 2024

yhakbar Oct 7, 2024

levkohimins Oct 7, 2024

levkohimins commented Oct 7, 2024

levkohimins commented Oct 7, 2024

denis256 left a comment

denis256 Oct 8, 2024

levkohimins Oct 8, 2024

	// 1. If the context cacnel contains a cause with a signal, this means that Terragrunt received the signal from the OS,
	// 1. If the context cancel contains a cause with a signal, this means that Terragrunt received the signal from the OS,

Suppressing 423 error in Terragrunt Provider Cache #3453

Suppressing 423 error in Terragrunt Provider Cache #3453

Conversation

levkohimins commented Oct 3, 2024

Description

TODOs

Release Notes (draft)

Migration Guide

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

levkohimins Oct 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

denis256 commented Oct 7, 2024

denis256 commented Oct 7, 2024

yhakbar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

levkohimins Oct 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

levkohimins commented Oct 7, 2024

levkohimins commented Oct 7, 2024

denis256 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Suppressing `423` error in Terragrunt Provider Cache #3453

Suppressing `423` error in Terragrunt Provider Cache #3453

levkohimins Oct 4, 2024 •

edited

Loading

levkohimins Oct 8, 2024 •

edited

Loading