Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Engine log level propagation #3443

Merged
merged 21 commits into from
Oct 1, 2024
Merged

Engine log level propagation #3443

merged 21 commits into from
Oct 1, 2024

Conversation

denis256
Copy link
Member

@denis256 denis256 commented Sep 26, 2024

Description

Included changes:

  • added propagation of log level to the engine through env variables
  • added env var documentation
  • added tests to check log passing
  • updated handling of env vars through flags
  • simplified tests

#3103

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 denis256 marked this pull request as ready for review September 26, 2024 16:52
yhakbar
yhakbar previously approved these changes Sep 26, 2024
@@ -97,6 +97,12 @@ To disable this feature, set the environment variable:
export TG_ENGINE_SKIP_CHECK=0
```

To configure a custom log level for the engine, set the `TG_ENGINE_LOG_LEVEL` environment variable to one of: `debug`, `info`, `warn`, `error`, `fatal`, or `panic`.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@denis256 , Please remove fatal and panic here. We removed those as valid log levels in Terragrunt, so it's a good idea to remove them from the docs here too. We'll probably eventually support stdout and stderr as valid log levels.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed panic and fatal.

engine/engine.go Outdated
@@ -518,10 +519,17 @@ func createEngine(terragruntOptions *options.TerragruntOptions) (*proto.EngineCl

terragruntOptions.Logger.Debugf("Creating engine %s", localEnginePath)

engineLogLevel := os.Getenv(EngineLogLevelEnv)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider whether you need special behavior to handle this:

&cli.BoolFlag{
Name: TerragruntLogDisableFlagName,
EnvVar: TerragruntLogDisableEnvName,
Usage: "Disable logging",
Action: func(ctx *cli.Context, _ bool) error {
opts.ForwardTFStdout = true
opts.Logger.SetOptions(log.WithFormatter(&format.SilentFormatter{}))
return nil
},
},

We might want to suppress logging in the engine too when it's disabled in Terragrunt.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added setting of "off" log level

engineLogLevel = terragruntOptions.LogLevel.String()
// turn off log formatting if disabled for terragrunt
if terragruntOptions.DisableLogFormatting {
engineLogLevel = hclog.Off.String()
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is different. Disabling log formatting only makes the logs output in key-value pairs, it doesn't stop them from logging altogether.

engine/engine.go Outdated
@@ -518,10 +519,21 @@ func createEngine(terragruntOptions *options.TerragruntOptions) (*proto.EngineCl

terragruntOptions.Logger.Debugf("Creating engine %s", localEnginePath)

engineLogLevel := os.Getenv(EngineLogLevelEnv)
Copy link
Contributor

@levkohimins levkohimins Sep 27, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It will not work with, false, 0, and will not notify about a wrong value. The same with TG_ENGINE_CACHE_PATH, TG_ENGINE_SKIP_CHECK.

In this particular case I would recommend adding opts.DisableLog

		&cli.BoolFlag{
			Name:   TerragruntLogDisableFlagName,
			EnvVar: TerragruntLogDisableEnvName,
			Destination: &opts.DisableLog,

Let's agree that the only entry point for all environment variables is CLI flags, this will help avoid issues in the future, when we forget about the existence of this "orphan", and it will live its own life. For example, when globally renaming a prefix or redesign the CLI interface. If for some reason you don't need to show the evn variable flag in the CLI help, you can simply hide it from users with Hidden: true, but they all will be in one place, and we will know about them.

Copy link
Contributor

@levkohimins levkohimins left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great job!

@denis256 denis256 merged commit e3a1cf8 into main Oct 1, 2024
5 checks passed
@denis256 denis256 deleted the engine-log-level branch October 1, 2024 14:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants