Default to forward slash-separated paths for path translation #2145
Conversation
pietern
requested review from
andrewnester,
shreyas-goenka and
denik
as code owners
pietern
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
pietern
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
pietern
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
pietern
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
| @@ -774,8 +773,8 @@ func TestTranslatePathJobEnvironments(t *testing.T) { | |||
| diags := bundle.Apply(context.Background(), b, mutator.TranslatePaths()) | |||
| require.NoError(t, diags.Error()) | |||
|
|
|||
| assert.Equal(t, strings.Join([]string{".", "job", "dist", "env1.whl"}, string(os.PathSeparator)), b.Config.Resources.Jobs["job"].JobSettings.Environments[0].Spec.Dependencies[0]) | |||
| assert.Equal(t, strings.Join([]string{".", "dist", "env2.whl"}, string(os.PathSeparator)), b.Config.Resources.Jobs["job"].JobSettings.Environments[0].Spec.Dependencies[1]) | |||
| assert.Equal(t, "./job/dist/env1.whl", b.Config.Resources.Jobs["job"].JobSettings.Environments[0].Spec.Dependencies[0]) | |||
There was a problem hiding this comment.
question - might not be in scope for this PR - sometimes the paths have ./ in front ("./job/dist/env1.whl"), sometimes they don't ("job/dist/task.jar").
Should we standardize on that? Why not drop "./" from everywhere.
There was a problem hiding this comment.
If I recall correctly, this has its roots in the need to differentiate between a path reference and a PyPI package.
Agree that we should revisit and see if it can be normalized.
@andrewnester Do you recall the specifics?
There was a problem hiding this comment.
Originally I think that was the reason but now I think we can safely drop it
pietern
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
| if errors.Is(err, fs.ErrNotExist) { | ||
| if filepath.Ext(localFullPath) != notebook.ExtensionNone { | ||
| if path.Ext(localFullPath) != notebook.ExtensionNone { |
There was a problem hiding this comment.
do we need to perform similar clean up across the whole codebase, or at least in bundle?
(main) ~/work/cli % git grep filepath bundle | wc -l
507
There was a problem hiding this comment.
There are a few other places where we do the same (e.g. sync includes), but definitely not everywhere.
The filepath package must be used for relative path computations because on Windows it makes sure it doesn't escape the drive or UNC path. I recall that also for globbing the input paths need to use the platform-native separator or they don't work.
pietern
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
Changes
This came up in #2122 where relative library paths showed up with backslashes on Windows. It's hard to run acceptance tests where paths may be in either form. This change updates path translation logic to always use forward slash-separated paths, including for absolute paths.
Tests
filepathpackage for path manipulation. The functions in this package always normalize their inputs to be platform-native paths.