Stop convert windows path to posix style #7826
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
github-actions
bot
added
area/local/start-api
Contributor
|
Hi @y-stm, thank you for raising this PR! I see some tests are failing due to linting issues, can you please update this PR to fix them? |
y-stm
force-pushed
the
stop-convert-windows-path-to-posix-style
branch
2 times, most recently
from
8dbca2b to
fc11bf2
Compare
Contributor
|
Can you elaborate on this following statement?
|
roger-zhangg
removed
the
stage/needs-triage
y-stm
force-pushed
the
stop-convert-windows-path-to-posix-style
branch
from
38d2277 to
4b9cc01
Compare
Author
|
@vicheey oh I'm sorry for word-typo. It means Docker Toolbox, not Dropbox. I also rebased the branch onto upstream/develop and run |
y-stm
force-pushed
the
stop-convert-windows-path-to-posix-style
branch
3 times, most recently
from
292362a to
76cd63f
Compare
y-stm
force-pushed
the
stop-convert-windows-path-to-posix-style
branch
2 times, most recently
from
8f3818b to
8a0f784
Compare
Contributor
|
Thank you. Approved the workflow and waiting for test result. |
Contributor
|
We just need to make sure we test this properly in Windows (cmd, Powershell, Git bash, etc). But Docker Toolbox is deprecated since a few years ago, so it would be okay to drop support at this point https://github.com/docker-archive/toolbox |
y-stm
force-pushed
the
stop-convert-windows-path-to-posix-style
branch
from
9b1075f to
f6f736d
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Which issue(s) does this change fix?
#7807
Why is this change necessary?
This change make enable to work
sam local invokeorsam local start-apiwith some container management tools other than Docker Desktop in Windows (such as Rancher Desktop and Podman Desktop.)How does it address the issue?
The cause of disturbing other tools is converting windows path to posix style when binds host directory to container.
(that converts 'C:\path\to\built\Function' to '/c/path/to/built/Function')
However, this conversion suits only for Docker Toolkit and other docker engines don't handle correctly unix-style-path as host directory path.
Removing converting function and uses will fix this issue.
What side effects does this change have?
This change may affect only in windows. Because removed function works as if identity function when given path is already unix style.
After this PR, sam cli will lose Docker Toolbox support.
Mandatory Checklist
PRs will only be reviewed after checklist is complete
make prpassesmake update-reproducible-reqsif dependencies were changedBy submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.