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 bug when fetching filemanger to ./ #392

Merged
merged 8 commits into from
Dec 3, 2020
Merged

Conversation

anthmatic
Copy link
Contributor

Description and Context

A developer in the HS Slack community discovered an issue where you could not download all filemanager files to the current working directory (./). When constructing a local path, if the folder name argument wasn't passed, it failed.

@gcorne
Copy link
Contributor

gcorne commented Nov 30, 2020

This isn't working for me. It looks to be trying to fetch the files to / instead of resolving to the CWD. It also looks like we're missing some error handling somewhere.

image

@anthmatic
Copy link
Contributor Author

@gcorne I fixed the error handling, but I'm a little unsure what the desired behavior should be when fetching TO /. I would think if I wanted to fetch to the CWD, I would run hs filemanager fetch <remote> . I'm not aware of any places in the CLI where we treat / as anything other than the root directory.

@gcorne
Copy link
Contributor

gcorne commented Dec 1, 2020

@anthmatic the above example has a destination of ./ which is an alias for .. I think something is up with how we're resolving paths if it is resolving to / but didn't dig into it.

@anthmatic
Copy link
Contributor Author

@gcorne ah, that's my mistake, I missed the .. I also misunderstood how path.resolve() works, but that should be fixed up now.

@anthmatic anthmatic merged commit fdd7fcb into master Dec 3, 2020
@anthmatic anthmatic deleted the fix/filemanager-fetch-all branch December 3, 2020 16:51
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.

2 participants