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.

Sign up for GitHub

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

Jump to bottom

Fix join paths to remove duplicate separators #209

Merged
merged 4 commits into from
May 3, 2021
Merged

Fix join paths to remove duplicate separators #209

merged 4 commits into from
May 3, 2021

Conversation

nkoenig
Copy link
Contributor

@nkoenig nkoenig commented Apr 30, 2021

🦟 Bug fix

Summary

The joinPaths function would remove trailing slashes from the function arguments. This can cause downstream applications, like fuel tools using libzip, to treat a directory as a file.

Checklist

  • Signed all commits for DCO
  • Added tests
  • Updated documentation (as needed)
  • Updated migration guide (as needed)
  • codecheck passed (See contributing)
  • All tests passed (See test coverage)
  • While waiting for a review on your PR, please help review another open pull request to support the maintainers

@nkoenig nkoenig requested a review from mjcarroll as a code owner April 30, 2021 23:33
@github-actions github-actions bot added 🏰 citadel Ignition Citadel 🔮 dome Ignition Dome labels Apr 30, 2021
@chapulina chapulina mentioned this pull request Apr 30, 2021
Closed
chapulina
chapulina approved these changes Apr 30, 2021
View reviewed changes
Copy link
Contributor

@chapulina chapulina left a comment

Choose a reason for hiding this comment

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

LGTM. regex can be a bit slow though, so we need to be careful if using this function repeatedly.

Nate Koenig added 3 commits April 30, 2021 17:31
Fixing windows and adding improved tests
4a43551 
Signed-off-by: Nate Koenig <[email protected]>
Another windows test fix
0d2ab5e 
Signed-off-by: Nate Koenig <[email protected]>
Fix regex
2146a40 
Signed-off-by: Nate Koenig <[email protected]>
mjcarroll
mjcarroll approved these changes May 3, 2021
View reviewed changes
Copy link
Contributor

@mjcarroll mjcarroll left a comment

Choose a reason for hiding this comment

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

Came back clean other than known Homebrew warnings.

@mjcarroll mjcarroll merged commit b85431f into ign-common3 May 3, 2021
@mjcarroll mjcarroll deleted the fix_join_paths branch May 3, 2021 20:04
@nkoenig nkoenig mentioned this pull request May 6, 2021
Merged
7 tasks
mjcarroll added a commit that referenced this pull request May 10, 2021
@mjcarroll
Revert "Fix join paths to remove duplicate separators (#209)"
b247cd0 
This reverts commit b85431f.
@mjcarroll mjcarroll mentioned this pull request May 10, 2021
Closed
@scpeters scpeters mentioned this pull request May 13, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chapulina chapulina chapulina approved these changes

@mjcarroll mjcarroll mjcarroll approved these changes

Assignees
No one assigned
Labels
🏰 citadel Ignition Citadel 🔮 dome Ignition Dome
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@nkoenig @mjcarroll @chapulina