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

Included task envfile doesn't consider cwd #160

Closed
rjaduthie opened this issue Jul 27, 2023 · 8 comments
Closed

Included task envfile doesn't consider cwd #160

rjaduthie opened this issue Jul 27, 2023 · 8 comments
Assignees
Labels
bug Something isn't working

Comments

@rjaduthie
Copy link
Contributor

Summary

Circumstances

When including a task from another file definition - e.g. ./subproject/pyproject.toml - and using the cwd property in the root pyproject.toml.

Effect

If an envfile is used in the task definition in the submodule project, the path to the envfile doesn't consider the cwd when it's imported.

To recreate

Assuming there's an executable my_task and an env file ./subproject/.env.my_task:

./pyproject.toml:

[[tool.poe.include]]
path = "subproject/pyproject.toml"
cwd = "subproject"

./subproject/pyproject.toml

[tool.poe.tasks.my_task]
envfile = ".env.my_task"
cmd = "my_task"

Execution of poetry run poe my_task in the root project will have the variables as defined in ./subproject/.env.my_task.

Possible solutions

I've had a bit of a look around the code. Possibly when adding envs to the EnvVarsManager, the cwd can be respected for include'd tasks?

The code is quite complex (... I haven't yet worked out the interaction with the _get_upstream_invocations etc.), so I haven't got a PR for this issue. I might have so time to look later, but hope that I can get some pointers to how to approach this fix.

@rjaduthie rjaduthie reopened this Jul 27, 2023
@rjaduthie
Copy link
Contributor Author

(I thought I found that the issue was with a virtualenv, hence closing, but it actually seems not.)

@nat-n
Copy link
Owner

nat-n commented Aug 5, 2023

Hi @rjaduthie, thanks for raising this!

I see how that could be surprising. If I understand correctly what you're demonstrating then I guess the solution would be to make this line do something a bit like this one. You want to have go at it?

One complication with this however is that one might indeed wish for included tasks to be executed in a different directory, but still manage all the local (non checked in) .env files in the project root. So it be nice if we could work out how to have it both ways. Some ideas:

  1. always search both locations with a certain precedence
  2. support templating env vars into the value for envfile, e.g. envfile = "${POE_PROJECT}/.env" or create a new env var to also support envfile = "${POE_INCLUDE}/.env"
    I suspect the first option makes sense for your use case? but we could do both... What do you think?

@nat-n nat-n added the bug Something isn't working label Aug 5, 2023
@roger-duthie-pivotal
Copy link

Thanks for the hints. I'll try to take a look at this at some point and suggest a PR to fix.

In a follow up, I think there's a similar effect with the specification of commands as well. Using the example from my original post, if the subproject task uses a sequence, then the cwd directive is not observed - i.e.

[tool.poe.tasks.my_task]
sequence = 
[
  reference_task,
  { cmd = "scripts_dir_within_subproject/my_task.sh" },
]

... here the second task within the sequence is within a subdir of the subproject, but Poe doesn't "include" it with the correct cwd.

@roger-duthie-pivotal
Copy link

To respond to your question about the design options:

I wonder how obvious/intuitive it could be to a user if implementing the first option.

The second option could also get tricky, if you want to override an inherited behaviour.

A third option might be to have a flag within a task or the include block that specifies which behaviour to use - e.g. use_original_envfile=true - or something similarly suitable.

@nat-n
Copy link
Owner

nat-n commented Oct 8, 2023

@rjaduthie FYI I've started looking into this. It's more complex than I realised and I think justifies some refactor of how tasks manage and pass certain types of configuration.

@nat-n nat-n self-assigned this Oct 8, 2023
nat-n added a commit that referenced this issue Oct 8, 2023
Also:
- apply correction if cwd value passed to app is a file path
- begin refactor of how configuration is managed across tasks
- improve error handling
nat-n added a commit that referenced this issue Oct 8, 2023
Also:
- apply correction if cwd value passed to app is a file path
- begin refactor of how configuration is managed across tasks
- improve error handling
nat-n added a commit that referenced this issue Nov 4, 2023
Also:
- apply correction if cwd value passed to app is a file path
- begin refactor of how configuration is managed across tasks
- improve error handling
@nat-n
Copy link
Owner

nat-n commented Nov 4, 2023

The issue with sequence task members inheriting cwd should be fixed now with v0.24.2

I'm still working on a fix for the issue included task files which involves a larger refactor.

@roger-duthie-pivotal
Copy link

Just to say, we started using the updated package and it now works well for setting the working directory in sequenced tasks. Thank you for that fix!

@nat-n
Copy link
Owner

nat-n commented Apr 27, 2024

The 0.26.0 release includes a minor breaking change that addresses the original issue. The directory specified by the cwd option when including config will now be used at the base path for resolving relative paths for envfiles referenced within the included file.

The docs have been updated to reflect this.

@nat-n nat-n closed this as completed Apr 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants