Skip to content

chore(terraform): option to pass in instanced logger #8738

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

Merged
merged 7 commits into from
Apr 16, 2025

Conversation

Emyrk
Copy link
Contributor

@Emyrk Emyrk commented Apr 16, 2025

Description

Create new option OptionWithLogger to pass in an instanced logger, rather than using the global. By default, the global is still used.

There is 1 behavior change. The key module=<module_name> is passed to the evaluator and other child components (like the resolver). I intentionally kept this, as it helps log the current scope of the recursive nature of the parsing.

Eval logs before
2025-04-15T20:05:03-05:00	DEBUG	[terraform evaluator] Starting submodules evaluation...
2025-04-15T20:05:03-05:00	DEBUG	[terraform evaluator] Evaluating submodule	name="echo"
2025-04-15T20:05:03-05:00	DEBUG	[terraform evaluator] Starting module evaluation...	path="echo"
2025-04-15T20:05:03-05:00	DEBUG	[terraform evaluator] Starting iteration	iteration=0
2025-04-15T20:05:03-05:00	DEBUG	[terraform evaluator] Starting iteration	iteration=1
Eval logs after
2025-04-15T20:02:16-05:00	DEBUG	[terraform evaluator] Starting submodules evaluation...	module="root"
2025-04-15T20:02:16-05:00	DEBUG	[terraform evaluator] Evaluating submodule	module="root" name="echo"
2025-04-15T20:02:18-05:00	DEBUG	[terraform evaluator] Starting module evaluation...	module="echo" path="echo"
2025-04-15T20:02:18-05:00	DEBUG	[terraform evaluator] Starting iteration	module="echo" iteration=0
2025-04-15T20:02:18-05:00	DEBUG	[terraform evaluator] Starting iteration	module="echo" iteration=1
2025-04-15T20:02:18-05:00	DEBUG	[terraform evaluator] Context unchanged	module="echo" iteration=1
2025-04-15T20:02:18-05:00	DEBUG	[terraform evaluator] Starting post-submodules evaluation...	module="echo"

Checklist

  • I've read the guidelines for contributing to this repository.
  • I've followed the conventions in the PR title.
  • I've added tests that prove my fix is effective or that my feature works.
  • I've updated the documentation with the relevant information (if needed).
  • I've added usage information (if the PR introduces new options)
  • I've included a "before" and "after" example to the description (if the PR is a user interface change).

Emyrk added 5 commits April 15, 2025 18:39
Allow use of instanced logger over the global default. Add unit
test to attempt to catch any misuse of the global logger
@Emyrk Emyrk requested review from simar7 and nikpivkin as code owners April 16, 2025 01:07
Copy link
Member

@simar7 simar7 left a comment

Choose a reason for hiding this comment

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

lgtm, thanks for the very detailed tests! @nikpivkin could you also take a look?

@simar7
Copy link
Member

simar7 commented Apr 16, 2025

@Emyrk I think we will have to update the tests.

Copy link
Contributor

@nikpivkin nikpivkin left a comment

Choose a reason for hiding this comment

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

LGTM. Need to fix a assertion in one test after a logger update.

@nikpivkin
Copy link
Contributor

nikpivkin commented Apr 16, 2025

It seems that for nested modules the module attribute is repeated, which can be confusing:

2025-04-16T11:45:29+06:00       DEBUG   [terraform evaluator] Starting post-submodules evaluation...    module="root" module="a" module="b"
2025-04-16T11:45:29+06:00       DEBUG   [terraform evaluator] Starting iteration        module="root" module="a" module="b" iteration=0
2025-04-16T11:45:29+06:00       DEBUG   [terraform evaluator] Starting iteration        module="root" module="a" module="b" iteration=1

What if module contains the full name of the module, for example, module="root.a.b"?

@nikpivkin nikpivkin self-requested a review April 16, 2025 05:51
module path is also included in output logs now
@Emyrk
Copy link
Contributor Author

Emyrk commented Apr 16, 2025

@Emyrk I think we will have to update the tests.

My mistake, updated. The test is a nice example of the new module key in the logs:

d019ac7

What if module contains the full name of the module, for example, module="root.a.b"?

@nikpivkin I had thought about that too. We do not store the Parent parser as a field, just children. So it would require some extra code to get the full context of the Parent hierarchy from the newModuleParser function.

.With("module", p.moduleName + "." + moduleName) is only the immediate parent. It does not include the parents of your parent.

@Emyrk Emyrk requested a review from simar7 April 16, 2025 14:33
@simar7
Copy link
Member

simar7 commented Apr 16, 2025

@Emyrk I think we will have to update the tests.

My mistake, updated. The test is a nice example of the new module key in the logs:

d019ac7

What if module contains the full name of the module, for example, module="root.a.b"?

@nikpivkin I had thought about that too. We do not store the Parent parser as a field, just children. So it would require some extra code to get the full context of the Parent hierarchy from the newModuleParser function.

.With("module", p.moduleName + "." + moduleName) is only the immediate parent. It does not include the parents of your parent.

I think we can tackle this in a future PR since we don't store the Parent anyway today.

@simar7 simar7 added this pull request to the merge queue Apr 16, 2025
Merged via the queue into aquasecurity:main with commit 6c6beea Apr 16, 2025
12 checks passed
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