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

Relative path for module prefix #2716

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

denis256
Copy link
Member

@denis256 denis256 commented Sep 15, 2023

Description

Included changes:

  • Add setting of relative path for modules / absolute path for app

Fixes #2529.

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
Copy link
Member Author

Example output:

INFO[0000] The stack at /home/denis/projects/gruntwork/terragrunt/test/fixture-include-parent/app will be processed in the following order for command init:
Group 1
- Module /home/denis/projects/gruntwork/terragrunt/test/fixture-include-parent/dependency

Group 2
- Module /home/denis/projects/gruntwork/terragrunt/test/fixture-include-parent/app
 
[../dependency] 
[../dependency] Initializing the backend...
[../dependency] 
[../dependency] Initializing provider plugins...
[../dependency] 
[../dependency] Terraform has been successfully initialized!
[../dependency] 
[../dependency] You may now begin working with Terraform. Try running "terraform plan" to see
[../dependency] any changes that are required for your infrastructure. All Terraform commands
[../dependency] should now work.
[../dependency] 
[../dependency] If you ever set or change modules or backend configuration for Terraform,
[../dependency] rerun this command to reinitialize your working directory. If you forget, other
[../dependency] commands will detect it and remind you to do so if necessary.
[/home/denis/projects/gruntwork/terragrunt/test/fixture-include-parent/app] 
[/home/denis/projects/gruntwork/terragrunt/test/fixture-include-parent/app] Initializing the backend...
[/home/denis/projects/gruntwork/terragrunt/test/fixture-include-parent/app] 
[/home/denis/projects/gruntwork/terragrunt/test/fixture-include-parent/app] Initializing provider plugins...
[/home/denis/projects/gruntwork/terragrunt/test/fixture-include-parent/app] 
[/home/denis/projects/gruntwork/terragrunt/test/fixture-include-parent/app] Terraform has been successfully initialized!
[/home/denis/projects/gruntwork/terragrunt/test/fixture-include-parent/app] 
[/home/denis/projects/gruntwork/terragrunt/test/fixture-include-parent/app] You may now begin working with Terraform. Try running "terraform plan" to see
[/home/denis/projects/gruntwork/terragrunt/test/fixture-include-parent/app] any changes that are required for your infrastructure. All Terraform commands
[/home/denis/projects/gruntwork/terragrunt/test/fixture-include-parent/app] should now work.
[/home/denis/projects/gruntwork/terragrunt/test/fixture-include-parent/app] 
[/home/denis/projects/gruntwork/terragrunt/test/fixture-include-parent/app] If you ever set or change modules or backend configuration for Terraform,
[/home/denis/projects/gruntwork/terragrunt/test/fixture-include-parent/app] rerun this command to reinitialize your working directory. If you forget, other
[/home/denis/projects/gruntwork/terragrunt/test/fixture-include-parent/app] commands will detect it and remind you to do so if necessary.

Comment on lines +334 to +348
prefix := modulePath
workingDirPath := terragruntOptions.WorkingDir

// try to convert working dir to absolute path
if path, err := filepath.Abs(workingDirPath); err == nil {
workingDirPath = path
}

if workingDirPath != modulePath {
// try to generate relative path to module to be used as prefix
if relativePrefix, err := filepath.Rel(workingDirPath, modulePath); err == nil {
prefix = relativePrefix
}
}
opts.OutputPrefix = fmt.Sprintf("[%v] ", prefix)
Copy link
Contributor

@maunzCache maunzCache Oct 3, 2023

Choose a reason for hiding this comment

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

Edit: Just noticed this PR was in draft mode after spending an hour on it. Sorry if that comment came to early.

I was thinking about this part real hard because i thought there was another way and dug through some go implementation and came up with this:

Suggested change
prefix := modulePath
workingDirPath := terragruntOptions.WorkingDir
// try to convert working dir to absolute path
if path, err := filepath.Abs(workingDirPath); err == nil {
workingDirPath = path
}
if workingDirPath != modulePath {
// try to generate relative path to module to be used as prefix
if relativePrefix, err := filepath.Rel(workingDirPath, modulePath); err == nil {
prefix = relativePrefix
}
}
opts.OutputPrefix = fmt.Sprintf("[%v] ", prefix)
// try to convert working dir to absolute path
absoluteWorkingDirPath, err := filepath.Abs(terragruntOptions.WorkingDir); error != nil {
return _, err
}
modulePathPrefix := modulePath
if absoluteWorkingDirPath != modulePathPrefix {
// try to generate relative path to module to be used as prefix
modulePathPrefix, err = filepath.Rel(absoluteWorkingDirPath, modulePathPrefix); error != nil {
return _, err
}
}
opts.OutputPrefix = fmt.Sprintf("[%v] ", modulePathPrefix)

Let's go through this:

  1. The prefix var doesn't matter until later so move that down.
  2. If filepath.Abs or filepath.Rel errors, this happens on a system level or the input was garbage to begin with. Simply call it directly and pass the error
  3. prefix and modulePath should not differ during the runtime so pass it directly.

Also renamed the variables. Ignored it if its too verbose.

Note: I have not scrolled through the whole function so returning _ on error might be wrong.

So what do you think? I know it is a simple piece of code and i don't want to argue. Just exchanging thoughts.

@RafaelWO
Copy link

Hey, what is the status here? Looking forward to this being merged as it would improve the readability of the logs a lot 🙂

@yhakbar
Copy link
Collaborator

yhakbar commented Aug 13, 2024

Hey, what is the status here? Looking forward to this being merged as it would improve the readability of the logs a lot 🙂

@RafaelWO

I think it's likely that we're gonna close this PR soon.

Take a look at #3338 for another take on this feature.

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.

Enhancement request: TERRAGRUNT_INCLUDE_MODULE_PREFIX strip pwd
4 participants