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

Windows path fix #1190

Merged
merged 5 commits into from
Sep 15, 2023
Merged

Windows path fix #1190

merged 5 commits into from
Sep 15, 2023

Conversation

jribbink
Copy link
Contributor

@jribbink jribbink commented Sep 8, 2023

Closes #1183

Description

This fixes bugs related to Windows path formatting. We were using the "path" package in a few places that should have been using "path/filepath".

Some issues with this were more theoretical and didn't seem to surface yet (absolute paths like C:/foo/bar would not be recognized as absolute). But most critically the mixing of "path" and "path/filepath" meant that when doing comparisons/dictionary lookups to see if paths were equivalent the different format would result in two paths not matching (this is what broke deployment).

The PR also ensures that all paths written to flow.json are in UNIX format for cross-platform compatibility. This is the pattern adopted by NPM in the package.json file.


For contributor use:

  • Targeted PR against master branch
  • Linked to Github issue with discussion and accepted design OR link to spec that describes this work
  • Code follows the standards mentioned here
  • Updated relevant documentation
  • Re-reviewed Files changed in the Github PR explorer
  • Added appropriate labels

@codecov-commenter
Copy link

codecov-commenter commented Sep 8, 2023

Codecov Report

Patch coverage: 25.00% and no project coverage change.

Comparison is base (eee3195) 38.40% compared to head (819981c) 38.40%.
Report is 7 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #1190   +/-   ##
=======================================
  Coverage   38.40%   38.40%           
=======================================
  Files          38       38           
  Lines        1940     1940           
=======================================
  Hits          745      745           
  Misses       1107     1107           
  Partials       88       88           
Flag Coverage Δ
unittests 38.40% <25.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Changed Coverage Δ
internal/super/setup.go 0.00% <0.00%> (ø)
internal/super/files.go 12.35% <25.00%> (ø)
internal/test/test.go 38.93% <33.33%> (ø)

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@jribbink jribbink marked this pull request as draft September 11, 2023 04:48
Makefile Outdated Show resolved Hide resolved
Copy link
Contributor

@sideninja sideninja left a comment

Choose a reason for hiding this comment

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

Did a quick review. Great work. Left some comments. The most important feedback I have is to add a test that validates the persisting and loading of different path styles (unix, windows)

@jribbink jribbink marked this pull request as ready for review September 15, 2023 17:24
@jribbink jribbink merged commit cf41263 into master Sep 15, 2023
6 checks passed
@jribbink jribbink deleted the jribbink/windows-path-fix branch September 15, 2023 22:14
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.

flow project deploy broken on windows
4 participants