Repository navigation
fix: don't fail hybrid jobs when input symlinks are changed - #1362
Open
moneytosms wants to merge 1 commit into
Open
moneytosms wants to merge 1 commit into
moneytosms wants to merge 1 commit into
Conversation
clean_links checked link.is_symlink without calling it, so readlink ran on paths the job deleted or replaced, failing the job and dropping its result. Also init links before try so setup errors aren't masked by UnboundLocalError.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1362 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 171 171
Lines 11667 11667
Branches 1539 1539
=========================================
Hits 11667 11667 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
This branch has not been deployed
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Issue #, if available:
Closes #1363
Description of changes:
clean_linksin_entry_point_template.pycheckslink.is_symlinkwithout calling it, so the check is always true andreadlink()runs on every path. If the job deletes or replaces one of its linked input files (e.g. rewritesdata.csvin place),readlink()raises inside thefinallyblock. The job then fails even though the function returned fine, andsave_job_resultnever runs, so the result is lost.Also,
linkswas only assigned inside thetry, so ifos.chdirorlink_input()failed, thefinallyraisedUnboundLocalErrorand buried the real error.link.is_symlink()links = {}before thetryexcept:in the directory cleanup toexcept OSError:Testing done:
clean_linkswith a deleted link and a link replaced by a regular file, and for thelink_input()failure case. All three fail on main (FileNotFoundError,OSError: [Errno 22] Invalid argument,UnboundLocalError) and pass with the fix.results.jsonis written; with the fix the result is saved.tox -e unit-tests(100% coverage)ruff checkandruff format --checkMerge Checklist
Put an
xin the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. We're here to help! This is simply a reminder of what we are going to look for before merging your pull request.General
Tests
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.