Skip to content

fix: set process working dir based on relative path of -f file too #296

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

ryantm
Copy link
Contributor

@ryantm ryantm commented Jan 9, 2025

Why

  • extends now sets the working dir of the processes in it based on the relative path of the process-compose file.
  • The documentation says that -f should be equivalent to using extends
  • However, -f does not set the working dir of the process to match the process-compose file's path
  • To make the documentation consistent again, we should also set the working dir for the -f filename case.

What changed

  • Move the working dir copy from loadExtendProject to loadProjectFromFile
  • Update test

Test plan

  • -f and extends both set the expected working dir of the processes in my local tests

Why
===
* extends now sets the working dir of the processes in it based on the
relative path of the process-compose file.
* The documentation says that -f should be equvalent to using extends
* However, -f does not set the workign dir of the process to match the
process-compose file's path
* To make the documentation consistent again, we should also set the
working dir for the -f filename case.

What changed
===
* Move the working dir copy from loadExtendProject to
loadProjectFromFile

Test plan
===
* -f and extends both set the expected working dir of the processes in
my local tests
@ryantm ryantm force-pushed the rtm-01-08-fix-working-dir-cli branch from e50ae1a to 8b0f635 Compare January 9, 2025 01:10
Copy link

sonarqubecloud bot commented Jan 9, 2025

@ryantm
Copy link
Contributor Author

ryantm commented Jan 18, 2025

@F1bonacc1 gentle nudge here. Are you reticent about merging this because it breaks some backward compatibility with working dirs of CLI args?

@ryantm
Copy link
Contributor Author

ryantm commented Feb 19, 2025

@F1bonacc1 Just checking in again, it's been a month. Any feedback for this?

@F1bonacc1
Copy link
Owner

Hi @ryantm,
Sorry for the long absence. I had some personal issues to take care of.
On one hand, this change makes sense and I want to add it, on the other hand, it's a backward compatibility breaking change.
Although the documentation says that extends loading is equal to the -f flag, it doesn't mean that the -f flag is equal in every sense to extends.

@ryantm
Copy link
Contributor Author

ryantm commented Mar 4, 2025

Makes sense! Can we do a breaking change version release to get this? What other mechanisms can we use to gradually move people toward this world of sanity and consistency?

@F1bonacc1
Copy link
Owner

F1bonacc1 commented Mar 4, 2025

I see 3 options, I can't say I love any of them, but I am of course open for other suggestions:

  1. Break and communicate it - PC 2.0, the users won't be happy, but like you said, it brings us to a more consistent place.
  2. Adding another flag, maybe something like -af (for absolute file) that will implement the new convention.
  3. Another helper flag that will go along with -f, maybe -A, will treat all the file locations as absolute.

There may be other options that I haven't considered...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants