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(job_attachments): handle case-insensitive path extraction on Windows #287

Merged
merged 1 commit into from
Apr 7, 2024

Conversation

gahyusuh
Copy link
Contributor

@gahyusuh gahyusuh commented Apr 6, 2024

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

On Windows, asset paths are not treated as case-insensitive, leading to incorrect grouping of assets with paths that differ only in case. For example, if two paths "C:\Username\user\test1.txt" and "c:\uSerNaMe\user\test2.txt" exist, their shared root path should be extracted as "C:\Username\user". However, currently, these paths are treated as having different root paths due to the case-sensitivity issue.

What was the solution? (How)

The _get_asset_groups function has been modified to ensure that the root path extraction is case-insensitive on Windows.

What is the impact of this change?

Resolves the issue of incorrect asset grouping on Windows due to case-sensitivity

How was this change tested?

  • A new unit test case has been added to validate the case-insensitive root path extraction.
  • Existing unit tests and integration tests pass successfully.
  • Manual end-to-end testing: submitting a job with a following parameter:
    name: AssetsDir1
    type: PATH
    objectType: DIRECTORY
    dataFlow: IN
    default: ./INPUTS_1
    

Where the actual input directory's name on the file system is inputs_1. The job submission and job execution on a worker was working, and generated output files 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.

@gahyusuh gahyusuh marked this pull request as ready for review April 6, 2024 23:52
@gahyusuh gahyusuh requested a review from a team as a code owner April 6, 2024 23:52
@gahyusuh gahyusuh force-pushed the gahyusuh/win_case_insensitive branch 3 times, most recently from af21e7d to d857b1a Compare April 7, 2024 00:51
loachri
loachri previously approved these changes Apr 7, 2024
Copy link

@loachri loachri left a comment

Choose a reason for hiding this comment

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

LGTM! Ship it :shipit:

ttblanchard
ttblanchard previously approved these changes Apr 7, 2024
@gahyusuh gahyusuh dismissed stale reviews from ttblanchard and loachri via 3a91918 April 7, 2024 01:27
@gahyusuh gahyusuh force-pushed the gahyusuh/win_case_insensitive branch from d857b1a to 3a91918 Compare April 7, 2024 01:27
loachri
loachri previously approved these changes Apr 7, 2024
@gahyusuh gahyusuh force-pushed the gahyusuh/win_case_insensitive branch 3 times, most recently from 930d69f to daf9c7c Compare April 7, 2024 02:19
Signed-off-by: Gahyun Suh <132245153+gahyusuh@users.noreply.github.com>
@gahyusuh gahyusuh force-pushed the gahyusuh/win_case_insensitive branch from daf9c7c to c2d70c1 Compare April 7, 2024 02:31
@gahyusuh gahyusuh merged commit 7c3cc3d into mainline Apr 7, 2024
15 checks passed
@gahyusuh gahyusuh deleted the gahyusuh/win_case_insensitive branch April 7, 2024 02:43
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