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(cli): correct download-output command displaying resolved UNC path #337

Merged
merged 2 commits into from
May 30, 2024

Conversation

godobyte
Copy link
Contributor

@godobyte godobyte commented May 29, 2024

What was the problem/requirement? (What/Why)

The download-output CLI command can output resolved UNC root paths for mapped drive, leading to confusing logs like below

PS C:\Users\yuanbian\Git\deadline\deadline-cloud> deadline job download-output --job-id job-f5c23912e3b946799f9f18d30765847c
Downloading output from Job 'blender-3.5-splash'

Summary of files to download:
    S:\Blender\Output (2 files)

You are about to download files which may come from multiple root directories. Here are a list of the current root directories:
[0] \\DESKTOP-XXXXX\SD\Blender
> Please enter the index of root directory to edit, y to proceed without changes, or n to cancel the download (0, y, n) [y]: 0
> Please enter the new root directory path, or press Enter to keep it unchanged [\\DESKTOP-4LN5768\SD\Blender]: S:\Blender\Output

Summary of files to download:
    \\DESKTOP-XXXXX\SD\Blender\Output\Output (2 files)

You are about to download files which may come from multiple root directories. Here are a list of the current root directories:
[0] \\DESKTOP-XXXXX\SD\Blender\Output
> Please enter the index of root directory to edit, y to proceed without changes, or n to cancel the download (0, y, n) [y]: 0

What was the solution? (How)

Remove path resolve that output UNC path for mapped network drive.

What is the impact of this change?

The download-output command shows unresolved path for mapped drive.

PS C:\Users\yuanbian\Git\deadline\deadline-cloud> hatch run default:deadline job download-output --job-id job-f5c23912e3b946799f9f18d30765847c
Downloading output from Job 'blender-3.5-splash'

Summary of files to download:
    S:\Blender\Output (2 files)

You are about to download files which may come from multiple root directories. Here are a list of the current root directories:
[0] S:\Blender
> Please enter the index of root directory to edit, y to proceed without changes, or n to cancel the download (0, y, n) [y]: 0
> Please enter the new root directory path, or press Enter to keep it unchanged [S:\Blender]: S:\Blender\Out

Summary of files to download:
    S:\Blender\Out\Output (2 files)

You are about to download files which may come from multiple root directories. Here are a list of the current root directories:
[0] S:\Blender\Out
> Please enter the index of root directory to edit, y to proceed without changes, or n to cancel the download (0, y, n) [y]: y

How was this change tested?

  • All unit tests passed successfully.
  • Done some manual end-to-end tests including cross-platform scenarios, confirming that it worked as expected.
    • Fix Test
      • Downloading outputs from job with mapped drive on Windows: shows mapped drive letter (used to be UNC format path)
    • Regression Test
      • Downloading outputs from job with UNC format root paths on Windows: shows UNC format root paths
      • Downloading outputs from job with C drive root paths on windows: shows C drive root paths
      • Downloading outputs from job with UNC format root paths on Linux: prompt OS mismatch and ask user to select local path
      • Downloading outputs from job with C drive root paths on Linux: prompt OS mismatch and ask user to select local path
      • Downloading outputs from job with Linux root paths on Linux: download as expected

Was this change documented?

No.

Is this a breaking change?

No.


By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

Signed-off-by: Godot Bian <13778003+godobyte@users.noreply.github.com>
@godobyte godobyte requested a review from a team as a code owner May 29, 2024 17:24
Signed-off-by: Godot Bian <13778003+godobyte@users.noreply.github.com>
Copy link

sonarcloud bot commented May 29, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

Copy link
Contributor

@marofke marofke left a comment

Choose a reason for hiding this comment

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

Great testing coverage Godot, looks good to me!

🚢 🦈

Copy link
Contributor

@chocecil chocecil left a comment

Choose a reason for hiding this comment

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

@godobyte godobyte changed the title fix(cli): download-output command display resolved download path fix(cli): correct download-output command display resolved download path May 29, 2024
@godobyte godobyte changed the title fix(cli): correct download-output command display resolved download path fix(cli): correct download-output command displays resolved download path May 29, 2024
@godobyte godobyte changed the title fix(cli): correct download-output command displays resolved download path fix(cli): correct download-output command to display download path May 29, 2024
@godobyte godobyte changed the title fix(cli): correct download-output command to display download path fix(cli): correct download-output command displaying resolved UNC path May 29, 2024
@godobyte
Copy link
Contributor Author

Thanks for reviewing the PR @chocecil !

That's good to know, I'll be careful with commit message and description at the time of squash merging.

  • No unit tests changed?

All existing unit tests are passing as expected.

  • Do we plan to add corresponding integ tests?

Integ tests might be a good fit to validate this change since it requires special drive setup in windows, but we should definitely have integ tests for the workflow to make sure there is no regression.

@chocecil
Copy link
Contributor

Thanks for reviewing the PR @chocecil !

That's good to know, I'll be careful with commit message and description at the time of squash merging.

  • No unit tests changed?

All existing unit tests are passing as expected.

  • Do we plan to add corresponding integ tests?

Integ tests might be a good fit to validate this change since it requires special drive setup in windows, but we should definitely have integ tests for the workflow to make sure there is no regression.

I mean we changed the code, but how come no unit tests needs to update?

@godobyte
Copy link
Contributor Author

godobyte commented May 30, 2024

I mean we changed the code, but how come no unit tests needs to update?

@chocecil If you mean by no added unit tests to validate, that's because this is an edge case on windows with mapped network drive. I'm not sure how we can mock the drive setup in unit test. Without that part, the unit test will just becomes a normal case on windows such as any other lettered drive, which is covered already. I can create a separate task to see what would be the best test strategy to cover this edge case.

@godobyte godobyte merged commit 6bcd9a2 into aws-deadline:mainline May 30, 2024
16 checks passed
@godobyte godobyte deleted the unc-path-resolve-fix branch May 30, 2024 17:29
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.

4 participants