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

feat: capture files with time-based variables for job attachments #162

Merged
merged 1 commit into from
Jun 7, 2024

Conversation

epmog
Copy link
Contributor

@epmog epmog commented May 31, 2024

Resolves #125

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

The deadliine-cloud rop node was evaluating parms at the current frame. So any filepaths that contained time-based variables would only have 1 of those files captured rather than the whole sequence

What was the solution? (How)

There were a few solutions available, each with various downsides.

  1. Evaluate the parm with evalAsStringAtFrame`, would require us to figure out all the relevant frame numbers and adjust for offsets.

    • this requires us to get it perfectly correct - which is definitely higher effort
  2. Grab the directory before the frame variable

    • anecdotal evidence seems to indicate frames are mostly in the file name, but users are not restricted to having them in directories. So automatically uploading whole directories without insight as to why they're added leads to the potential of a lot of extraneous files uploaded.
  3. Replace the relevant variables with globs and and let the stdlib take care of finding them.

I went with replacing the time-based variables for input filenames with globs to allow us to do some naive pattern matching. The code itself is then pretty simple with avenues to improve it later.

  • Downside here is there may be inadvertent files that get included because they match the glob pattern and not the time variable. Future improvement could be to filter out files that matched the globs but not the pattern
  • Input directories are being ignored in this change, since the solution can be equivalent to going up a directory, which can be pretty dangerous while multiple levels of directories down from frames seem like a pretty rare case.

What is the impact of this change?

We get a LOT more of the files users want to be included with the job!

How was this change tested?

Added a bunch of unit tests to capture the pattern replacement and globbing functionality.

hatch run fmt
hatch build
hatch run lint
hatch run test

Test coverage is now at ~67%

Required test coverage of 65.0% reached. Total coverage: 67.59%

I'll need to retest the node within Houdini again after some rebasing.

Was this change documented?

N/A

Is this a breaking change?

Fixing!


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

@epmog epmog force-pushed the filepaths-with-frames branch 4 times, most recently from 7900b4a to d910e88 Compare May 31, 2024 18:46
crowecawcaw
crowecawcaw previously approved these changes Jun 3, 2024
@epmog epmog force-pushed the filepaths-with-frames branch 2 times, most recently from efb5e0d to 4965bf2 Compare June 6, 2024 22:53
@epmog epmog marked this pull request as ready for review June 6, 2024 22:56
@epmog epmog requested a review from a team as a code owner June 6, 2024 22:56
@epmog epmog force-pushed the filepaths-with-frames branch from 4965bf2 to c171c14 Compare June 6, 2024 22:57
Signed-off-by: Morgan Epp <60796713+epmog@users.noreply.github.com>
@epmog epmog force-pushed the filepaths-with-frames branch from c171c14 to 5e77d20 Compare June 6, 2024 22:58
Copy link

sonarqubecloud bot commented Jun 6, 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

@epmog epmog added the enhancement New feature or request label Jun 7, 2024
@epmog epmog merged commit 55d1a6b into aws-deadline:mainline Jun 7, 2024
16 checks passed
@epmog epmog deleted the filepaths-with-frames branch June 7, 2024 14:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bug: filepaths with $F token in the input are not expanded to include all files for a frame range
3 participants