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

feat: add "recurse-dependencies" option #3118

Conversation

michaeldelago
Copy link

@michaeldelago michaeldelago commented May 2, 2024

Description

This change reverts to the pre 0.55.6 behavior of not recursing dependencies for all operations. In case a user desires this behavior, they can enable it with --recurse-dependencies or TERRAGRUNT_RECURSE_DEPENDENCIES.

The changes in this PR should mitigate the performance issues noticed in #2980

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 --recurse-dependencies flag. This flag recursively parses dependencies instead of parsing only the original terragrunt module's dependencies.

Migration Guide

n/a

Copy link

sonarcloud bot commented May 10, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
No data about Duplication

See analysis details on SonarCloud

@michaeldelago
Copy link
Author

Updated to latest version of master

Copy link

sonarcloud bot commented Jul 30, 2024

@yhakbar
Copy link
Collaborator

yhakbar commented Jul 30, 2024

Thanks for the PR @michaeldelago . I'm discussing internally with the team to see if this is the right approach for this.

My concern is that the behavior you're introducing here is a breaking change, so we need to make sure it makes sense to have the default be that recursive parsing of configurations isn't done.

If we determine that recursive parsing is something that all users should expect to be done by default, I'll ask that you flip the default behavior of the flag so that users have to opt-in to recursive parsing. If it's the case that we think this is an edge case that users aren't likely to need and that it's acceptable for this to be opt-in, we'll accept your PR, assuming everything else looks good.

@yhakbar
Copy link
Collaborator

yhakbar commented Jul 30, 2024

@michaeldelago

Discussed this with the team, and there are problems with introducing this breaking change.

Say you have the following dependency chain:
A <-- B <-- C

Where A depends on B, which depends on C.

In this example, if B re-exports an input from dependency C as an output to A, A will either get an empty value or a mock value from the dependency on C.

This is non-obvious behavior that shouldn't be the default experience for users.

For this reason, please update the default to recursively parse dependencies.

This new functionality that you're introducing also does not come with any new tests. To make sure that this behavior is well documented, and the edge cases are tested, please introduce tests that test this behavior both with the flag set, and without it.

Thanks!

@yhakbar
Copy link
Collaborator

yhakbar commented Aug 9, 2024

Hey @michaeldelago ,

To avoid having this PR go stale, I'm going to close it out. Please feel free to open it again after addressing the feedback so far.

@yhakbar yhakbar closed this Aug 9, 2024
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.

2 participants