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

Fix chDirPath option for absolute paths #240

Merged
merged 2 commits into from
May 24, 2022

Conversation

jukie
Copy link
Collaborator

@jukie jukie commented May 24, 2022

Closes #222
There's no need to add a prefix in the code because the current path would already be respected by default. In turn, having this prefix breaks absolute paths during execution (i.e. tfswitch -c /some/other/path) as described in #222 and demonstrated below.

Current behavior:

### works correctly 
$ ./tfswitch -c test-data/test_tfswitchtoml
Reading configuration from current directory for .tfswitch.toml
Switched terraform to version "0.11.3" 

### chDirPath is incorrectly being read as: /Users/Jukie/terraform-switcher//Users/Jukie/terraform-switcher/test-data/test_tfswitchtoml
$ ./tfswitch -c $PWD/test-data/test_tfswitchtoml
Use the arrow keys to navigate: ↓ ↑ → ← 
? Select Terraform version: 
  ▸ 0.11.3 *recent
    1.2.1
    1.2.0
    1.1.9
↓   1.1.8

After fixes:

### still works
$ ./tfswitch -c test-data/test_tfswitchtoml
Reading configuration from current directory for .tfswitch.toml
Switched terraform to version "0.11.3" 

### chDirPath is correctly being read as: /Users/Jukie/terraform-switcher/test-data/test_tfswitchtoml
$ ./tfswitch -c $PWD/test-data/test_tfswitchtoml
Reading configuration from current directory for .tfswitch.toml
Switched terraform to version "0.11.3" 

@jukie
Copy link
Collaborator Author

jukie commented May 24, 2022

CC @yermulnik @warrensbox

Copy link
Collaborator

@yermulnik yermulnik left a comment

Choose a reason for hiding this comment

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

LGTM
@warrensbox has there been any background for the previous implementation which prevented use of absolute paths with chdir cmdline opt that we're not aware of? Or this PR is good to get merged?

@jukie
Copy link
Collaborator Author

jukie commented May 24, 2022

I tried testing with every combination of specifying directories I could think of to break things after my changes but I think this was just a bug.

@yermulnik
Copy link
Collaborator

yermulnik commented May 24, 2022

I tried testing with every combination of specifying directories I could think of to break things after my changes but I think this was just a bug.

Yep, I can't think of any sensible use case also given that escaping from current working directory wasn't restricted. Let's wait for Warren's input overnight (by my TZ) and merge tomorrow if there're no objections from him (I'd like to ask to remind me about this PR tomorrow so that I merge it. thanks).

@warrensbox warrensbox merged commit 5aaf943 into warrensbox:feature/release-0.14-API May 24, 2022
@jukie jukie deleted the fix-chDirPath branch May 24, 2022 22:54
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.

None yet

3 participants