-
Notifications
You must be signed in to change notification settings - Fork 886
Fixing Facade Error on commit counting #3123
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
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Sean P. Goggins <s@goggins.com>
Signed-off-by: Sean P. Goggins <s@goggins.com>
Signed-off-by: Sean P. Goggins <s@goggins.com>
Signed-off-by: Sean P. Goggins <s@goggins.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copilot reviewed 1 out of 1 changed files in this pull request and generated no comments.
Comments suppressed due to low confidence (1)
augur/tasks/git/util/facade_worker/facade_worker/utilitymethods.py:192
- The new implementation of get_repo_commit_count is being commented out, leaving an older version active. Consider removing the old, commented-out code to avoid potential confusion and ensure that the intended improvements are actually in effect.
"""
Signed-off-by: Sean P. Goggins <s@goggins.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
Signed-off-by: Sean P. Goggins <s@goggins.com>
Signed-off-by: Sean P. Goggins <s@goggins.com>
Latest update is fixing some issues and the task is usually succeeding:
|
Signed-off-by: Sean P. Goggins <s@goggins.com>
Signed-off-by: Sean P. Goggins <s@goggins.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
"invalid index-pack" errors. | ||
""" | ||
pack_dir = os.path.join(repo_loc, "objects", "pack") | ||
pack_files = glob(os.path.join(pack_dir, "pack-*.pack")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ensure that the glob module is imported to avoid runtime errors in the remove_corrupted_pack_files function.
Copilot uses AI. Check for mistakes.
f"Retry of git rev-list failed in {repo_loc} with return code " | ||
f"{retry_error.returncode}: {retry_stderr}" | ||
) | ||
return 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Returning 0 on retry failure may mask underlying repository issues; consider propagating the error instead for clearer error handling.
return 0 | |
raise retry_error |
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A reasonable point. investigating the issues logged here right now.
Signed-off-by: Sean P. Goggins <s@goggins.com>
Signed-off-by: Sean P. Goggins <s@goggins.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
# For other return codes, re-raise the error. | ||
raise initial_error | ||
|
||
except Exception as e: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] Catching a broad Exception may mask specific error conditions. It is recommended to catch more specific exceptions to handle unexpected errors more accurately.
except Exception as e: | |
except (OSError, ValueError) as e: |
Copilot uses AI. Check for mistakes.
Signed-off-by: Sean P. Goggins <s@goggins.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
augur/tasks/git/util/facade_worker/facade_worker/_old_method.py:1
- [nitpick] The entire file is commented out legacy code. If it’s no longer needed, consider removing it to prevent confusion and reduce maintenance overhead.
'''
pack_files = glob(os.path.join(pack_dir, "pack-*.pack")) | ||
idx_files = glob(os.path.join(pack_dir, "pack-*.idx")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similarly, use 'glob.glob' instead of 'glob' to correctly obtain the index files.
pack_files = glob(os.path.join(pack_dir, "pack-*.pack")) | |
idx_files = glob(os.path.join(pack_dir, "pack-*.idx")) | |
pack_files = glob.glob(os.path.join(pack_dir, "pack-*.pack")) | |
idx_files = glob.glob(os.path.join(pack_dir, "pack-*.idx")) |
Copilot uses AI. Check for mistakes.
Signed-off-by: Sean P. Goggins <s@goggins.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
Comments suppressed due to low confidence (1)
augur/tasks/git/util/facade_worker/facade_worker/utilitymethods.py:147
- The glob function is being used directly despite importing the entire 'glob' module. To avoid runtime errors, either import the glob function using 'from glob import glob' or call it with the module reference as 'glob.glob(...)'.
pack_files = glob(os.path.join(pack_dir, "pack-*.pack"))
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
remove_corrupted_pack_files(repo_loc, logger) | ||
|
||
# If the error message indicates a bad remote reference, attempt to fix it. | ||
bad_ref = "refs/remotes/origin/dependabot/docker/test/IntegrationTests/docker/postgres-16.4" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The bad_ref value is hard-coded. Consider making this value configurable or documenting why this specific reference is targeted to ensure future maintainability.
bad_ref = "refs/remotes/origin/dependabot/docker/test/IntegrationTests/docker/postgres-16.4" | |
bad_ref = get_bad_ref() |
Copilot uses AI. Check for mistakes.
…s.py Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Signed-off-by: Sean P. Goggins <s@goggins.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
Comments suppressed due to low confidence (2)
augur/tasks/git/util/facade_worker/facade_worker/utilitymethods.py:230
- Ensure that 'check_output' and 'CalledProcessError' are explicitly imported from the subprocess module to avoid runtime errors if they are not imported elsewhere.
output = check_output(
augur/tasks/git/util/facade_worker/facade_worker/_old_method.py:1
- [nitpick] Consider removing or archiving this legacy _old_method file if it is no longer needed to reduce code clutter and maintenance overhead.
'''
Signed-off-by: Sean P. Goggins <s@goggins.com>
Signed-off-by: Sean P. Goggins <s@goggins.com>
Signed-off-by: Sean P. Goggins <s@goggins.com>
Signed-off-by: Sean Goggins <outdoors@acm.org>
Description
This PR fixes that issue.