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

Always resolve relative include paths relative to the including Taskfile #823

Conversation

theunrepentantgeek
Copy link
Contributor

Enforces consistency by always resolving relative filepath references to Taskfiles from the directory containing the parent Taskfile.

Resolves #822

Previously, relative filepath references were resolved multiple times, leading to some unanticipated behaviour.

To illustrate, consider the following directory structure:

  • /path/to/project/Taskfile.yaml # top level
  • /path/to/project/alpha/Taskfile.yaml # first subfolder
  • /path/to/project/common/Taskfile.yaml # second subfolder, a sibling of alpha

With the following includes

# /path/to/project/Taskfile.yaml
includes:
  alpha:
    taskfile: ./alpha/Taskfile.yaml
    dir: ./alpha
  common: 
    taskfile: ./common/Taskfile.yaml
    dir: ./common
# /path/to/project/alpha/Taskfile.yaml
includes:
  common: 
    taskfile: ../common/Taskfile.yaml
    dir: ../common

When loading /path/to/project/alpha/Taskfile.yaml, the include ../common/Taskfile.yaml is correctly being resolved as /path/to/project/common/Taskfile.yaml.

But when loading the top level Taskfile /path/to/project/Taskfile.yaml, that same import (../common/Taskfile.yaml) is being resolved a second time, as /path/to/common/Taskfile.yaml, leading to an error.

This fix modifies all includes to use absolute paths, ensuring the correct Taskfile is read.

@theunrepentantgeek theunrepentantgeek marked this pull request as draft July 26, 2022 01:30
@theunrepentantgeek
Copy link
Contributor Author

Pulling back to draft status while I fix the failing tests.

Comment on lines -78 to +90
path, err := execext.Expand(includedTask.Taskfile)
tf, err := includedTask.FullTaskfilepath()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This diff makes it look as though execext.Expand() has been removed - it's still present, just lower down on line #95.

@theunrepentantgeek
Copy link
Contributor Author

I think this is now ready for review.

One note. Instead of doing something "clever" to ensure error messages retained their use of relative paths, I opted to update the tests to reflect the fully-qualified paths that now show up. Keeping things simple seemed the smarter option.

@theunrepentantgeek theunrepentantgeek marked this pull request as ready for review July 27, 2022 03:38
@andreynering
Copy link
Member

Hi @theunrepentantgeek, thanks a lot for your contribution!

I merged this with a few changes at e396f4d.

I added a test and changed Expand to to run before converting the path to absolute. (Expand is there to make include paths like ~/taskfiles, i.e. the user home directory, work).

@theunrepentantgeek theunrepentantgeek deleted the fix/taskfile-relative-paths branch August 5, 2022 02:11
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.

Relative paths to included taskfiles not consistently resolved
2 participants