chore: use double quotes consistently#2405
chore: use double quotes consistently#2405fgallott wants to merge 2 commits intoopenshift-eng:mainfrom
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
WalkthroughThis pull request standardizes string literal quoting across the codebase by converting single-quoted strings to double-quoted strings in Python files. Additionally, several scattered functional changes introduce new parameters and logic modifications in a few modules. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
openshift-merge-robot
added
the
needs-rebase
fgallott
force-pushed
the
improvements
branch
from
c7c7ba9 to
ea2d58e
Compare
openshift-merge-robot
removed
the
needs-rebase
fgallott
force-pushed
the
improvements
branch
from
ea2d58e to
7cd48a0
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
art-cluster/cloudfront/mirror/lambda_r2_art-srv-request-basic-auth.py (1)
134-160:⚠️ Potential issue | 🟡 MinorFix ValueError on malformed pocket usernames to return clean unauthorized response.
username.index("+")raisesValueErrorif the '+' is missing, which will propagate uncaught and produce a 5xx error instead of a clean unauthorized response. The code has no exception handler to catch this.Replace with
partition()to safely extract the pocket name without throwing:Proposed fix
- if username.index("+") > 0: + pocket_name, sep, _ = username.partition("+") + if sep and pocket_name: if not POCKET_SERVICE_ACCOUNTS: POCKET_SERVICE_ACCOUNTS = get_secrets_manager_secret_dict( "art_srv_request_basic_auth/POCKET_SERVICE_ACCOUNTS" ) - pocket_name = username.split("+")[0] if uri.startswith(f"/pockets/{pocket_name}/"): if username in POCKET_SERVICE_ACCOUNTS: if hmac.compare_digest(password, POCKET_SERVICE_ACCOUNTS[username]): authorized = Truedoozer/doozerlib/brew.py (1)
50-80:⚠️ Potential issue | 🟡 MinorReplace bare
exceptwithexcept Exceptionto avoid masking interrupts during long polls.Bare
except:catchesBaseExceptionsubclasses includingKeyboardInterruptandSystemExit, which should not be suppressed. Since the exception handling is specifically for transient connection issues inwatcher.update(), useexcept Exception:instead.🛠️ Suggested fixes
Line 67 in
watch_task:- except: + except Exception: except_count += 1 # possible for watcher.update() to except during connection issue, try again log_f("watcher.update() exception. Trying again in 60s.\n{}".format(traceback.format_exc()))Line 125 in
watch_tasks:- except: + except Exception: except_counts[task_id] += 1 # possible for watcher.update() to except during connection issue, try again log_f("watcher.update() exception. Trying again in 60s.\n{}".format(traceback.format_exc()))Also applies to: 113-145
artcommon/artcommonlib/exectools.py (1)
237-271:⚠️ Potential issue | 🟡 MinorStdErr bytes are appended to stdout in realtime mode.
Line 270 adds stderr toout, leavingerrempty and mixing streams. This breaks callers that rely on stderr.🐛 Suggested fix
- out += error + err += errordoozer/doozerlib/backend/rebaser.py (1)
1394-1401:⚠️ Potential issue | 🟠 MajorGuard against missing USER in the final stage.
final_stage_usercan beNoneif the last stage has no USER instruction, which will raise an AttributeError on.split(). Please guard before splitting.✅ Suggested fix
- if final_stage_user.split()[-1] == "0": + if final_stage_user and final_stage_user.split()[-1] == "0": final_stage_user = None # Avoid redundant USER 0 statement after repo removal
🤖 Fix all issues with AI agents
In `@artcommon/artcommonlib/bigquery.py`:
- Around line 41-50: The except blocks that handle failures from
self.client.query(...).result() currently call self.logger.error("Failed
executing query %s: ", err) and log the error incorrectly (placing the exception
where the query should be) and omit the traceback; update both occurrences that
catch Exception as err to log the actual query and full traceback by including
the query variable in the message and passing exc_info=True (or use
self.logger.exception) so the stack trace is captured; keep the raise to
re-raise the exception after logging.
In `@artcommon/artcommonlib/runtime.py`:
- Line 60: Remove the trailing space from the log message so it reads "Konflux
DB initialized" instead of "Konflux DB initialized "; update the call to
self._logger.info in the runtime.py location where the Konflux DB is initialized
(the method containing self._logger.info) to use the trimmed string.
In `@doozer/doozerlib/cli/__main__.py`:
- Line 163: The conditional in __main__.py uses the method reference
output.lower instead of calling it, so the CSV branch never triggers; update the
check in the CLI output handling (the code around the elif that currently reads
output.lower) to call the method (e.g., output.lower() or
output.strip().lower()) before comparing to "csv", ensuring the CSV output
branch in the CLI dispatch logic (the elif in the main CLI function) can be
selected.
In `@doozer/doozerlib/cli/fbc.py`:
- Around line 335-371: The command registration for "beta:fbc:merge" (the
function decorated by `@cli.command`("beta:fbc:merge") in fbc.py) defines the
"--dry-run" Click option twice causing a parameter collision; remove the
duplicate Click option decorator (the second --dry-run occurrence) so only a
single click.option("--dry-run", ...) remains for the fbc_merge command and
ensure any help/default text you want is preserved on the kept decorator.
In `@doozer/doozerlib/cli/images_streams.py`:
- Around line 409-414: The print expression prints " " + stdout or stderr
which always picks the left operand even when stdout is an empty string, masking
stderr; update the print after exectools.cmd_assert (where stdout, stderr are
returned) to use a parenthesized fallback like " " + (stdout or stderr) so
stderr is used when stdout is empty; locate this change in images_streams.py
around the loop that calls exectools.cmd_assert and prints results (references:
exectools.cmd_assert, stdout, stderr, group_label).
In `@doozer/doozerlib/cli/inspect_stream.py`:
- Around line 56-57: The strict-mode messages in inspect_stream.py print the
literal "{issues}" instead of the variable contents; update the two print
statements that output "Running in strict mode; saw: {issues}" and the second
occurrence to use f-strings (prefix with f) so the issues variable is
interpolated (keep the existing exit(1) behavior); locate these prints in the
inspect_stream.py function that performs the strict-mode check and add the f
prefix to both string literals.
🧹 Nitpick comments (9)
artcommon/artcommonlib/util.py (1)
148-158: Consider using_for the unusedservervariable.The static analysis correctly flags that
serveris unpacked but never used. Since you're touching this area for formatting, consider this optional cleanup:🔧 Optional fix
`@retry`(reraise=True, wait=wait_fixed(10), stop=stop_after_attempt(3)) async def download_file_from_github(repository, branch, path, token: str, destination, session): - server, org, repo_name = split_git_url(repository) + _, org, repo_name = split_git_url(repository) url = f"https://raw.githubusercontent.com/{org}/{repo_name}/{branch}/{path}"art-cluster/cloudfront/mirror/lambda_r2_art-srv-enterprise-s3-get-index-html-gen.py (1)
120-121: Optional: Simplify redundantstr()conversion in f-string.The
str()call is redundant inside the f-string since f-strings perform automatic string conversion. Consider simplifying or using an explicit conversion flag.♻️ Suggested simplification
- print(f"Traversing dir {str(path_top_dir)}") + print(f"Traversing dir {path_top_dir}")Alternatively, use an explicit conversion flag if you want to make the string conversion explicit:
- print(f"Traversing dir {str(path_top_dir)}") + print(f"Traversing dir {path_top_dir!s}")art-cluster/cloudfront/mirror/lambda_art-srv-enterprise-s3-get-index-html-gen.py (1)
119-120: Optional: Simplify redundantstr()conversion in f-string.The
str()call is redundant inside the f-string since f-strings perform automatic string conversion. Consider simplifying or using an explicit conversion flag.♻️ Suggested simplification
- print(f"Traversing dir {str(path_top_dir)}") + print(f"Traversing dir {path_top_dir}")Alternatively, use an explicit conversion flag if you want to make the string conversion explicit:
- print(f"Traversing dir {str(path_top_dir)}") + print(f"Traversing dir {path_top_dir!s}")artcommon/artcommonlib/gitdata.py (1)
88-97: Mutable default argument detected by static analysis.Line 94 uses a mutable list as a default argument. While the current implementation safely creates a new list on line 118, the idiomatic pattern is to use
Noneand initialize within the function to avoid potential future issues.🔧 Suggested fix
def __init__( self, data_path=None, clone_dir="./", commitish="master", sub_dir=None, - exts=["yaml", "yml", "json"], + exts=None, reclone=False, logger=None, ):Then at line 118:
- self.exts = ["." + e.lower() for e in exts] + if exts is None: + exts = ["yaml", "yml", "json"] + self.exts = ["." + e.lower() for e in exts]doozer/doozerlib/cli/olm_bundle.py (1)
242-249: Consider usingLOGGER.exception()for cleaner exception logging.The current pattern uses
traceback.print_exc()(which prints to stderr) followed byLOGGER.error(). UsingLOGGER.exception()would log both the message and the traceback to the same logging destination in a single call.♻️ Proposed refactor
except Exception as err: - traceback.print_exc() - LOGGER.error("Error during rebase or build for: {}".format(operator_nvr)) + LOGGER.exception("Error during rebase or build for: %s", operator_nvr) record["message"] = str(err)doozer/doozerlib/cli/config.py (1)
674-675: Pre-existing: Broad exception handling.The static analyzer flags catching bare
Exceptionhere. While this is a pre-existing pattern and not introduced by this PR, consider narrowing the exception type in a future refactor to avoid masking unexpected errors.doozer/doozerlib/cli/config_plashet.py (1)
159-164: Pre-existing: HTTP request without timeout.The static analyzer flags this
requests.postcall without atimeoutparameter. While this is pre-existing and not introduced by this PR, missing timeouts can cause the process to hang indefinitely if the server doesn't respond. Consider adding a reasonable timeout in a future refactor.doozer/doozerlib/cli/__main__.py (1)
376-376: Pre-existing: Unused variableout.The static analyzer flags that
outis never used. Consider using_for the unused variable to make the intent explicit.♻️ Suggested fix
- rc, out, err = exectools.cmd_gather(cmd, realtime=True) + rc, _, err = exectools.cmd_gather(cmd, realtime=True)doozer/doozerlib/cli/__init__.py (1)
314-316: Pre-existing: Consider exception chaining.The static analyzer suggests using
raise ... from errto preserve the exception chain. While pre-existing, this would improve debuggability by showing the originalValueErrorin the traceback.♻️ Suggested improvement
except ValueError: - raise click.BadParameter("Expected integers in version fields") + raise click.BadParameter("Expected integers in version fields") from None
| self.logger.debug("Executing query: %s", query) | ||
|
|
||
| try: | ||
| results = self.client.query(query).result() | ||
| self.logger.debug('Query returned %s result rows', results.total_rows) | ||
| self.logger.debug("Query returned %s result rows", results.total_rows) | ||
| return results | ||
|
|
||
| except Exception as err: | ||
| self.logger.error('Failed executing query %s: ', err) | ||
| self.logger.error("Failed executing query %s: ", err) | ||
| raise |
There was a problem hiding this comment.
Log the failed query and include traceback.
Current error logs pass err where the query should be and omit stack traces. This makes failures harder to diagnose.
🛠️ Suggested fix
@@
- except Exception as err:
- self.logger.error("Failed executing query %s: ", err)
+ except Exception:
+ self.logger.exception("Failed executing query: %s", query)
raise
@@
- except Exception as err:
- self.logger.error("Failed executing query: %s", err)
+ except Exception:
+ self.logger.exception("Failed executing query: %s", query)
raiseAlso applies to: 57-66
🧰 Tools
🪛 Ruff (0.15.0)
[warning] 46-46: Consider moving this statement to an else block
(TRY300)
[warning] 49-49: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
🤖 Prompt for AI Agents
In `@artcommon/artcommonlib/bigquery.py` around lines 41 - 50, The except blocks
that handle failures from self.client.query(...).result() currently call
self.logger.error("Failed executing query %s: ", err) and log the error
incorrectly (placing the exception where the query should be) and omit the
traceback; update both occurrences that catch Exception as err to log the actual
query and full traceback by including the query variable in the message and
passing exc_info=True (or use self.logger.exception) so the stack trace is
captured; keep the raise to re-raise the exception after logging.
| try: | ||
| self.konflux_db = KonfluxDb(enable_cache=disable_konflux_db_cache is False) | ||
| self._logger.info('Konflux DB initialized ') | ||
| self._logger.info("Konflux DB initialized ") |
There was a problem hiding this comment.
Minor: Trailing space in log message.
The log message has a trailing space: "Konflux DB initialized ". Consider removing it for cleaner logs.
🧹 Proposed fix
- self._logger.info("Konflux DB initialized ")
+ self._logger.info("Konflux DB initialized")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| self._logger.info("Konflux DB initialized ") | |
| self._logger.info("Konflux DB initialized") |
🤖 Prompt for AI Agents
In `@artcommon/artcommonlib/runtime.py` at line 60, Remove the trailing space from
the log message so it reads "Konflux DB initialized" instead of "Konflux DB
initialized "; update the call to self._logger.info in the runtime.py location
where the Konflux DB is initialized (the method containing self._logger.info) to
use the trimmed string.
| @cli.command("beta:fbc:merge", short_help="Merge FBC fragments from multiple index images into a single FBC repository") | ||
| @pass_runtime | ||
| @click.option( | ||
| "--message", "-m", metavar='MSG', help="Commit message. If not provided, a default generated message will be used." | ||
| "--message", "-m", metavar="MSG", help="Commit message. If not provided, a default generated message will be used." | ||
| ) | ||
| @click.option("--dry-run", is_flag=True, default=False, help="Perform a dry run without pushing changes.") | ||
| @click.option( | ||
| "--dest-dir", metavar='DEST_DIR', default=None, help="The destination directory for the merged FBC fragments." | ||
| "--dest-dir", metavar="DEST_DIR", default=None, help="The destination directory for the merged FBC fragments." | ||
| ) | ||
| @click.option("--registry-auth", metavar='REGISTRY_AUTH', default=None, help="Path to the registry auth file.") | ||
| @click.option("--registry-auth", metavar="REGISTRY_AUTH", default=None, help="Path to the registry auth file.") | ||
| @click.option( | ||
| '--konflux-kubeconfig', metavar='PATH', help='Path to the kubeconfig file to use for Konflux cluster connections.' | ||
| "--konflux-kubeconfig", metavar="PATH", help="Path to the kubeconfig file to use for Konflux cluster connections." | ||
| ) | ||
| @click.option( | ||
| '--konflux-context', | ||
| metavar='CONTEXT', | ||
| help='The name of the kubeconfig context to use for Konflux cluster connections.', | ||
| "--konflux-context", | ||
| metavar="CONTEXT", | ||
| help="The name of the kubeconfig context to use for Konflux cluster connections.", | ||
| ) | ||
| @click.option( | ||
| '--konflux-namespace', | ||
| metavar='NAMESPACE', | ||
| help='The namespace to use for Konflux cluster connections. If not provided, will be auto-detected based on group (e.g., ocp-art-tenant for openshift- groups, art-oadp-tenant for oadp- groups).', | ||
| "--konflux-namespace", | ||
| metavar="NAMESPACE", | ||
| help="The namespace to use for Konflux cluster connections. If not provided, will be auto-detected based on group (e.g., ocp-art-tenant for openshift- groups, art-oadp-tenant for oadp- groups).", | ||
| ) | ||
| @click.option( | ||
| '--fbc-repo', | ||
| metavar='FBC_REPO', | ||
| help='The git repository to push the FBC to. Use FBC_GIT_USERNAME and FBC_GIT_PASSWORD environment variables to set credentials.', | ||
| "--fbc-repo", | ||
| metavar="FBC_REPO", | ||
| help="The git repository to push the FBC to. Use FBC_GIT_USERNAME and FBC_GIT_PASSWORD environment variables to set credentials.", | ||
| default=constants.ART_FBC_GIT_REPO, | ||
| ) | ||
| @click.option( | ||
| '--fbc-branch', | ||
| metavar='FBC_BRANCH', | ||
| "--fbc-branch", | ||
| metavar="FBC_BRANCH", | ||
| default=None, | ||
| help='The branch to push the FBC to. Defaults to art-<group>-fbc-stage', | ||
| help="The branch to push the FBC to. Defaults to art-<group>-fbc-stage", | ||
| ) | ||
| @click.option('--dry-run', default=False, is_flag=True, help='Do not build anything, but only print build operations.') | ||
| @click.option("--dry-run", default=False, is_flag=True, help="Do not build anything, but only print build operations.") | ||
| @click.option( |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for all --dry-run options in the fbc.py file with line numbers
rg -n --fixed-strings '@click.option("--dry-run"' doozer/doozerlib/cli/fbc.py -A 1Repository: openshift-eng/art-tools
Length of output: 488
🏁 Script executed:
#!/bin/bash
# Show the context around the --dry-run decorators to identify which function they decorate
sed -n '330,375p' doozer/doozerlib/cli/fbc.pyRepository: openshift-eng/art-tools
Length of output: 2168
🏁 Script executed:
#!/bin/bash
# Show lines 370-395 to see which function is being decorated
sed -n '370,395p' doozer/doozerlib/cli/fbc.pyRepository: openshift-eng/art-tools
Length of output: 1184
🏁 Script executed:
#!/bin/bash
# Let's verify the function signature to confirm both --dry-run options are for the same function
sed -n '387,395p' doozer/doozerlib/cli/fbc.pyRepository: openshift-eng/art-tools
Length of output: 361
🏁 Script executed:
#!/bin/bash
# Check if the function definition is indeed for fbc_merge
sed -n '335,395p' doozer/doozerlib/cli/fbc.py | grep -E "(`@cli.command`|async def fbc_merge)"Repository: openshift-eng/art-tools
Length of output: 208
🏁 Script executed:
#!/bin/bash
# Check which function the --dry-run at line 801 belongs to
sed -n '795,810p' doozer/doozerlib/cli/fbc.pyRepository: openshift-eng/art-tools
Length of output: 747
🏁 Script executed:
#!/bin/bash
# Find which command/function the --dry-run at line 801 belongs to
sed -n '770,810p' doozer/doozerlib/cli/fbc.py | grep -E "(`@cli.command`|async def)"Repository: openshift-eng/art-tools
Length of output: 166
🏁 Script executed:
#!/bin/bash
# Verify the exact --dry-run decorators for fbc_merge function
sed -n '340p;370p' doozer/doozerlib/cli/fbc.pyRepository: openshift-eng/art-tools
Length of output: 293
Remove duplicate --dry-run option to avoid Click parameter collision.
Lines 340 and 370 both define --dry-run for the fbc_merge command; Click will treat this as a duplicate option definition and cause command registration to fail. Keep one and remove the other.
🛠️ Suggested fix (remove duplicate at line 370)
-@click.option("--dry-run", default=False, is_flag=True, help="Do not build anything, but only print build operations.")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @cli.command("beta:fbc:merge", short_help="Merge FBC fragments from multiple index images into a single FBC repository") | |
| @pass_runtime | |
| @click.option( | |
| "--message", "-m", metavar='MSG', help="Commit message. If not provided, a default generated message will be used." | |
| "--message", "-m", metavar="MSG", help="Commit message. If not provided, a default generated message will be used." | |
| ) | |
| @click.option("--dry-run", is_flag=True, default=False, help="Perform a dry run without pushing changes.") | |
| @click.option( | |
| "--dest-dir", metavar='DEST_DIR', default=None, help="The destination directory for the merged FBC fragments." | |
| "--dest-dir", metavar="DEST_DIR", default=None, help="The destination directory for the merged FBC fragments." | |
| ) | |
| @click.option("--registry-auth", metavar='REGISTRY_AUTH', default=None, help="Path to the registry auth file.") | |
| @click.option("--registry-auth", metavar="REGISTRY_AUTH", default=None, help="Path to the registry auth file.") | |
| @click.option( | |
| '--konflux-kubeconfig', metavar='PATH', help='Path to the kubeconfig file to use for Konflux cluster connections.' | |
| "--konflux-kubeconfig", metavar="PATH", help="Path to the kubeconfig file to use for Konflux cluster connections." | |
| ) | |
| @click.option( | |
| '--konflux-context', | |
| metavar='CONTEXT', | |
| help='The name of the kubeconfig context to use for Konflux cluster connections.', | |
| "--konflux-context", | |
| metavar="CONTEXT", | |
| help="The name of the kubeconfig context to use for Konflux cluster connections.", | |
| ) | |
| @click.option( | |
| '--konflux-namespace', | |
| metavar='NAMESPACE', | |
| help='The namespace to use for Konflux cluster connections. If not provided, will be auto-detected based on group (e.g., ocp-art-tenant for openshift- groups, art-oadp-tenant for oadp- groups).', | |
| "--konflux-namespace", | |
| metavar="NAMESPACE", | |
| help="The namespace to use for Konflux cluster connections. If not provided, will be auto-detected based on group (e.g., ocp-art-tenant for openshift- groups, art-oadp-tenant for oadp- groups).", | |
| ) | |
| @click.option( | |
| '--fbc-repo', | |
| metavar='FBC_REPO', | |
| help='The git repository to push the FBC to. Use FBC_GIT_USERNAME and FBC_GIT_PASSWORD environment variables to set credentials.', | |
| "--fbc-repo", | |
| metavar="FBC_REPO", | |
| help="The git repository to push the FBC to. Use FBC_GIT_USERNAME and FBC_GIT_PASSWORD environment variables to set credentials.", | |
| default=constants.ART_FBC_GIT_REPO, | |
| ) | |
| @click.option( | |
| '--fbc-branch', | |
| metavar='FBC_BRANCH', | |
| "--fbc-branch", | |
| metavar="FBC_BRANCH", | |
| default=None, | |
| help='The branch to push the FBC to. Defaults to art-<group>-fbc-stage', | |
| help="The branch to push the FBC to. Defaults to art-<group>-fbc-stage", | |
| ) | |
| @click.option('--dry-run', default=False, is_flag=True, help='Do not build anything, but only print build operations.') | |
| @click.option("--dry-run", default=False, is_flag=True, help="Do not build anything, but only print build operations.") | |
| @click.option( | |
| `@cli.command`("beta:fbc:merge", short_help="Merge FBC fragments from multiple index images into a single FBC repository") | |
| `@pass_runtime` | |
| `@click.option`( | |
| "--message", "-m", metavar="MSG", help="Commit message. If not provided, a default generated message will be used." | |
| ) | |
| `@click.option`("--dry-run", is_flag=True, default=False, help="Perform a dry run without pushing changes.") | |
| `@click.option`( | |
| "--dest-dir", metavar="DEST_DIR", default=None, help="The destination directory for the merged FBC fragments." | |
| ) | |
| `@click.option`("--registry-auth", metavar="REGISTRY_AUTH", default=None, help="Path to the registry auth file.") | |
| `@click.option`( | |
| "--konflux-kubeconfig", metavar="PATH", help="Path to the kubeconfig file to use for Konflux cluster connections." | |
| ) | |
| `@click.option`( | |
| "--konflux-context", | |
| metavar="CONTEXT", | |
| help="The name of the kubeconfig context to use for Konflux cluster connections.", | |
| ) | |
| `@click.option`( | |
| "--konflux-namespace", | |
| metavar="NAMESPACE", | |
| help="The namespace to use for Konflux cluster connections. If not provided, will be auto-detected based on group (e.g., ocp-art-tenant for openshift- groups, art-oadp-tenant for oadp- groups).", | |
| ) | |
| `@click.option`( | |
| "--fbc-repo", | |
| metavar="FBC_REPO", | |
| help="The git repository to push the FBC to. Use FBC_GIT_USERNAME and FBC_GIT_PASSWORD environment variables to set credentials.", | |
| default=constants.ART_FBC_GIT_REPO, | |
| ) | |
| `@click.option`( | |
| "--fbc-branch", | |
| metavar="FBC_BRANCH", | |
| default=None, | |
| help="The branch to push the FBC to. Defaults to art-<group>-fbc-stage", | |
| ) |
🤖 Prompt for AI Agents
In `@doozer/doozerlib/cli/fbc.py` around lines 335 - 371, The command registration
for "beta:fbc:merge" (the function decorated by `@cli.command`("beta:fbc:merge")
in fbc.py) defines the "--dry-run" Click option twice causing a parameter
collision; remove the duplicate Click option decorator (the second --dry-run
occurrence) so only a single click.option("--dry-run", ...) remains for the
fbc_merge command and ensure any help/default text you want is preserved on the
kept decorator.
| print(f"Would have run: {cmd}") | ||
| continue | ||
| stdout, stderr = exectools.cmd_assert(cmd, retries=3) | ||
| print(' ' + stdout or stderr) | ||
| print(" " + stdout or stderr) | ||
| else: | ||
| print(f'No buildconfigs associated with this group: {group_label}') | ||
| print(f"No buildconfigs associated with this group: {group_label}") |
There was a problem hiding this comment.
stderr can be masked when stdout is empty.
" " + stdout or stderr always evaluates to the left string (even if empty), hiding stderr. Wrap the fallback in parentheses.
🛠️ Suggested fix
- print(" " + stdout or stderr)
+ print(" " + (stdout or stderr))🤖 Prompt for AI Agents
In `@doozer/doozerlib/cli/images_streams.py` around lines 409 - 414, The print
expression prints " " + stdout or stderr which always picks the left operand
even when stdout is an empty string, masking stderr; update the print after
exectools.cmd_assert (where stdout, stderr are returned) to use a parenthesized
fallback like " " + (stdout or stderr) so stderr is used when stdout is empty;
locate this change in images_streams.py around the loop that calls
exectools.cmd_assert and prints results (references: exectools.cmd_assert,
stdout, stderr, group_label).
| print("Running in strict mode; saw: {issues}") | ||
| exit(1) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, check if the file exists and has the problematic line
rg -n 'Running in strict mode; saw:' doozer/doozerlib/cli/inspect_stream.py
# Get a broader context around lines 56-57
sed -n '50,65p' doozer/doozerlib/cli/inspect_stream.pyRepository: openshift-eng/art-tools
Length of output: 751
Fix missing f-string interpolation in strict-mode message.
Lines 56 and 61 both print the literal {issues} instead of the variable contents. Both occurrences need the f prefix.
🛠️ Suggested fix
- print("Running in strict mode; saw: {issues}")
+ print(f"Running in strict mode; saw: {issues}")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| print("Running in strict mode; saw: {issues}") | |
| exit(1) | |
| print(f"Running in strict mode; saw: {issues}") | |
| exit(1) |
🤖 Prompt for AI Agents
In `@doozer/doozerlib/cli/inspect_stream.py` around lines 56 - 57, The strict-mode
messages in inspect_stream.py print the literal "{issues}" instead of the
variable contents; update the two print statements that output "Running in
strict mode; saw: {issues}" and the second occurrence to use f-strings (prefix
with f) so the issues variable is interpolated (keep the existing exit(1)
behavior); locate these prints in the inspect_stream.py function that performs
the strict-mode check and add the f prefix to both string literals.
fgallott
force-pushed
the
improvements
branch
from
7cd48a0 to
4f22350
Compare
|
@fgallott: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
doozer/doozerlib/cli/release_gen_assembly.py (1)
305-358:⚠️ Potential issue | 🟡 MinorHandle standard release names that include hyphens.
Line 346 uses
standard_release_name.split("-"), which breaks for versions containing additional hyphens (e.g.,4.8.0-rc.3-s390x). Usersplit("-", 1)and validate the format to avoidValueErroror mis-parsing.🛠️ Proposed fix
- version, brew_cpu_arch = standard_release_name.split("-") # 4.7.22-s390x => ['4.7.22', 's390x'] + try: + version, brew_cpu_arch = standard_release_name.rsplit("-", 1) # 4.7.22-s390x => ['4.7.22', 's390x'] + except ValueError: + self._exit_with_error( + f"Invalid --standard value {standard_release_name!r}; expected format <version>-<arch>" + )doozer/doozerlib/cli/release_calc_upgrade_tests.py (1)
24-29:⚠️ Potential issue | 🟡 MinorSampling logic doesn’t guarantee 5 evenly distributed versions.
remaining[step::step]skips the first middle item and yields a variable count (e.g., 4 when len=5, 8 when len=9), which diverges from the “5 equally distributed” intent.Suggested fix
- step = len(remaining) // 5 - test_edges += remaining[step::step] + step = max(1, len(remaining) // 5) + test_edges += remaining[0::step][:5]
🤖 Fix all issues with AI agents
In
`@art-cluster/cloudfront/mirror/lambda_art-srv-enterprise-s3-get-index-html-gen.py`:
- Around line 119-121: The print statement uses an unnecessary str() inside an
f-string (print(f"Traversing dir {str(path_top_dir)}")) which violates RUF010;
update the f-string to use the !s conversion flag instead (e.g., f"Traversing
dir {path_top_dir!s}") so you remove str() and still stringify path_top_dir;
change the call where VERBOSE is checked (the block that prints when VERBOSE is
true) to use the !s conversion.
In `@art-cluster/cloudfront/mirror/lambda_art-srv-request-basic-auth.py`:
- Around line 589-598: The authorization path handling pockets uses
username.index("+") which throws on missing "+"; change the check to use a safe
lookup (e.g., username.find("+") or `"+" in username`) so it returns -1/False
instead of raising; keep the surrounding logic that lazy-loads
POCKET_SERVICE_ACCOUNTS via get_secrets_manager_secret_dict and that extracts
pocket_name = username.split("+")[0] only when the separator exists and the
uri.startswith(f"/pockets/{pocket_name}/") check applies.
In `@art-cluster/cloudfront/mirror/lambda_r2_art-srv-request-basic-auth.py`:
- Around line 147-157: Change the unsafe username.index("+") check to a safe
existence check so missing "+" doesn’t raise ValueError and cause 500s: in the
code block that handles pocket access (symbols: POCKET_SERVICE_ACCOUNTS,
get_secrets_manager_secret_dict, pocket_name, and the
uri.startswith(f"/pockets/{pocket_name}/") branch) replace the if
username.index("+") > 0 check with a safe check such as if "+" in username and
username.find("+") > 0 (or username.split("+",1) length check), and ensure that
when the "+" is absent the code falls through to return a 401 rather than
raising an exception.
- Line 179: The conditional uses request.get("method", None).lower() which will
raise AttributeError if method is None; change the check in the elif that
references request.get("method", None).lower() to first obtain the method value
and guard it (either use a safe empty-string default or an explicit None check)
so lower() is only called on a string—e.g., read method = request.get("method")
and then test method is not None and method.lower() != "get" (or use (method or
"") before .lower()) to avoid the exception.
In `@doozer/doozerlib/backend/konflux_image_builder.py`:
- Around line 801-812: Change the two bare except blocks around the BigQuery
insert to catch only Exception (i.e. use "except Exception") instead of bare
"except"; in the inner handler (around
taskrun_db_client.client.insert_rows_json) and the outer handler, replace the
pattern logger.warning(...) followed by traceback.print_exc() with a single
logger.exception(...) call so the error message and stacktrace are logged
together; keep the existing pprint.pprint(rows) behavior in the inner handler
before re-raising if desired and reference the insert call
(taskrun_db_client.client.insert_rows_json), the rows variable, and the logger
calls when editing.
- Around line 219-231: The code assumes IMAGE_URL and IMAGE_DIGEST exist and
calls image_pullspec.split(...) which will raise if missing; update the block
that handles KonfluxBuildOutcome.SUCCESS in konflux_image_builder to validate
results before splitting: fetch results via
pipelinerun_info.to_dict().get("status", {}).get("results", []) and check that
IMAGE_URL and IMAGE_DIGEST values are present and non-empty (e.g., variables
image_pullspec and image_digest are not None); if either is missing, flip the
outcome to a non-releaseable state (e.g., set outcome =
KonfluxBuildOutcome.FAILURE or mark unreleasable and log the missing keys using
pipelinerun_info and record) instead of performing split(), and only compute
record["image_pullspec"] and record["image_tag"] when image_pullspec and
image_digest are valid.
In `@doozer/doozerlib/cli/config_plashet.py`:
- Around line 159-164: The POST requests to the Errata API in config_plashet.py
(the requests.post call that assigns to res when calling
/erratum/{advisory_id}/remove_build and the other POST at lines ~187-192) lack a
timeout and can hang indefinitely; update both requests.post invocations to
include a bounded timeout argument (e.g., timeout=10 or, better, a configurable
timeout value read from existing config or an added CLI option) so the calls
fail fast on network stalls and handle timeout exceptions accordingly in the
surrounding logic (keep the verify, auth, and json args intact and add timeout
to the same call sites).
In `@doozer/doozerlib/cli/fbc.py`:
- Around line 372-375: The help text for the CLI option "--plr-template"
contains a typo "fragement"; update the help string to use "fragment" wherever
this option is defined (the add_argument call that sets
default=constants.KONFLUX_DEFAULT_FBC_BUILD_PLR_TEMPLATE_URL and its duplicate
further down around the second definition). Locate the add_argument for
"--plr-template" and replace "build the FBC fragement" with "build the FBC
fragment" in both occurrences so the user-facing CLI help is corrected.
In `@doozer/doozerlib/cli/images_health.py`:
- Around line 20-24: The ConcernCode enum member LATEST_BUILD_SUCCEEDED has an
incorrect string value "LATEST_BUILT_SUCCEEDED" which will change serialized
concern codes; update the value of ConcernCode.LATEST_BUILD_SUCCEEDED to the
correct string "LATEST_BUILD_SUCCEEDED" so external consumers keep the expected
serialized token and tests/clients remain compatible.
In `@doozer/doozerlib/cli/images_okd.py`:
- Around line 713-727: Replace the bare except in the block that invokes
oc.invoke (using temp_dir, latest_pullspec, tag, payload_tag_names) with an
explicit "except Exception as e" so system-exiting signals aren't swallowed,
capture the exception in a variable (e) and set detail accordingly, and move the
cleanup call shutil.rmtree(temp_dir) into a finally block so temp_dir is always
removed after the try/except; ensure scos_status and detail logic remains
unchanged inside the try/except/finally structure.
In `@doozer/doozerlib/cli/images_streams.py`:
- Around line 727-746: The code uses insecure hashlib.md5 in calc_parent_digest
and compute_dockerfile_digest (and indirectly via extract_parent_digest);
replace hashlib.md5() with hashlib.sha256() in both calc_parent_digest and
compute_dockerfile_digest so digests use SHA-256 for change detection, and
ensure any places that compare these digests (callers of extract_parent_digest,
calc_parent_digest, compute_dockerfile_digest) continue to treat them as opaque
hex strings (no other logic depends on MD5 length). Alternatively, if you
intentionally want to keep MD5, add an explicit "# noqa: S324" on the
hashlib.md5() lines, but preferred fix is switching to hashlib.sha256().
🧹 Nitpick comments (11)
artcommon/artcommonlib/release_util.py (1)
88-90: Consider chaining the exception for better traceability.Static analysis (Ruff B904) flags that re-raising a different exception type inside an
exceptblock should usefromto preserve the original traceback context. This helps debugging by showing the originalKeyError.♻️ Suggested improvement
`@classmethod` def from_name(cls, phase_name): try: return cls[phase_name.upper().replace("-", "_")] - except KeyError: - raise ValueError(f"{phase_name} is not a valid phase name") + except KeyError as err: + raise ValueError(f"{phase_name} is not a valid phase name") from errdoozer/doozerlib/backend/konflux_olm_bundler.py (1)
367-367: Consider addingstrict=Truetozip()to catch length mismatches.The
zip()call here pairsreferenceswithimage_infos. If these collections ever have different lengths (e.g., due to a bug in the image info retrieval), the mismatch would be silently ignored. Addingstrict=Truewould raise aValueErrorif lengths differ, making such issues immediately visible.Proposed fix
- for pullspec, image_info in zip(references, image_infos): + for pullspec, image_info in zip(references, image_infos, strict=True):doozer/doozerlib/cli/config_tag_rpms.py (1)
221-233: Consider addressing Ruff TRY003 for exception messages.Ruff flags the long inline messages in the ValueError raises within this block. If you want to align with TRY003, consider encapsulating these messages in a small helper or using a custom exception to keep the raise sites concise.
artcommon/artcommonlib/exectools.py (1)
505-512: LGTM on quote changes; considerTypeErrorfor invalid argument type.The quote normalization is correct. The static analysis tool flags a pre-existing issue: Python convention is to raise
TypeError(notValueError) when a function receives an argument of an unexpected type.This is optional since it's pre-existing behavior not introduced by this PR:
♻️ Optional fix for exception type
- raise ValueError("Invalid type for manifest-tool options provided") + raise TypeError("Invalid type for manifest-tool options provided")doozer/doozerlib/cli/release_gen_payload.py (1)
635-635: Consider usingnext(iter(...))for first element access.Creating a list just to access the first element is slightly inefficient.
♻️ Suggested refactor
- multi_model_assembly_name = list(assembly_def["releases"].keys())[0] + multi_model_assembly_name = next(iter(assembly_def["releases"].keys()))doozer/doozerlib/brew.py (1)
66-79: LGTM on the quote normalization.The double-quote conversion is consistent with the PR objective.
However, static analysis flags the bare
except:on line 67 (pre-existing code). Since you're touching adjacent lines, consider tightening this to catch specific exceptions (e.g.,except Exception:) to avoid silently swallowingKeyboardInterruptorSystemExit.,
🔧 Optional improvement for adjacent pre-existing code
- except: + except Exception:doozer/doozerlib/cli/scan_sources_konflux.py (1)
1269-1270: Optional: Consider exception chaining for better debugging.This is a pre-existing pattern (not introduced by this PR), but for future reference: when re-raising a new exception from a caught one, using
raise ... from epreserves the original traceback context.💡 Suggested improvement
except Exception as e: - raise DoozerFatalError("Could not determine commitish for rpm %s: %s", rpm.rpm_name, e) + raise DoozerFatalError("Could not determine commitish for rpm %s: %s", rpm.rpm_name, e) from edoozer/doozerlib/cli/olm_bundle.py (1)
209-249: LGTM on quote changes. Pre-existing code patterns could be improved.The quote style changes are correct. Note that static analysis flags two pre-existing issues in this block that could be addressed in a follow-up:
- Line 242: Catching bare
Exceptionis broad; consider catching more specific exceptions if applicable.- Line 244:
LOGGER.exception()would automatically include the traceback, makingtraceback.print_exc()on line 243 redundant.♻️ Optional refactor for exception handling
except Exception as err: - traceback.print_exc() - LOGGER.error("Error during rebase or build for: {}".format(operator_nvr)) + LOGGER.exception("Error during rebase or build for: %s", operator_nvr) record["message"] = str(err),
doozer/doozerlib/build_info.py (1)
280-282: Consider defining a custom exception for invalid build system (optional).Static analysis flags TRY003 here — the recommendation is to define exception messages within a custom exception class rather than inline. This is a pre-existing style issue, not introduced by this PR.
class InvalidBuildSystemError(ValueError): def __init__(self, build_system: str): super().__init__(f"Invalid build system: {build_system}")This is a minor stylistic refinement that can be deferred.
artcommon/artcommonlib/git_helper.py (1)
88-88: Use list-unpacking with explicit list() conversion for git command construction.Ruff's RUF005 suggests
["git", *list(args)]over list concatenation. Thelist()wrapper is necessary becauseargsis typed asSequence[str], not a list.♻️ Proposed refactor
- return await exectools.cmd_assert_async(["git"] + list(args), check=check, env=set_env, stdout=stdout, **kwargs) + return await exectools.cmd_assert_async(["git", *list(args)], check=check, env=set_env, stdout=stdout, **kwargs)- return await exectools.cmd_gather_async(["git"] + list(args), check=check, env=set_env, **kwargs) + return await exectools.cmd_gather_async(["git", *list(args)], check=check, env=set_env, **kwargs)- return exectools.cmd_gather(["git"] + list(args), **kwargs) + return exectools.cmd_gather(["git", *list(args)], **kwargs)Also applies to: 104-104, 114-114
doozer/doozerlib/backend/konflux_image_builder.py (1)
346-347: Uselogging.exceptionto preserve the stack trace.This exception handler logs an error and re-raises;
logging.exception()automatically captures the full stack trace, making it clearer and more idiomatic thanLOGGER.error().♻️ Suggested tweak
except ChildProcessError: - LOGGER.error(f"Failed to fetch signature for {source_image_pullspec}") + LOGGER.exception(f"Failed to fetch signature for {source_image_pullspec}") raise
| if VERBOSE: | ||
| print(f'Traversing dir {str(path_top_dir)}') | ||
| print(f"Traversing dir {str(path_top_dir)}") | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n 'str\(path_top_dir\)' art-cluster/cloudfront/mirror/lambda_art-srv-enterprise-s3-get-index-html-gen.pyRepository: openshift-eng/art-tools
Length of output: 198
🏁 Script executed:
#!/bin/bash
# Check context around line 120
sed -n '118,122p' art-cluster/cloudfront/mirror/lambda_art-srv-enterprise-s3-get-index-html-gen.py
# Search for all str() calls inside f-strings (more comprehensive check)
rg -n 'f["\x27].*str\(' art-cluster/cloudfront/mirror/lambda_art-srv-enterprise-s3-get-index-html-gen.pyRepository: openshift-eng/art-tools
Length of output: 222
Use f-string conversion flags instead of str() to satisfy RUF010.
Replace the redundant str() call inside the f-string with the !s conversion flag (line 120):
Suggested fix
- print(f"Traversing dir {str(path_top_dir)}")
+ print(f"Traversing dir {path_top_dir!s}")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if VERBOSE: | |
| print(f'Traversing dir {str(path_top_dir)}') | |
| print(f"Traversing dir {str(path_top_dir)}") | |
| if VERBOSE: | |
| print(f"Traversing dir {path_top_dir!s}") | |
🧰 Tools
🪛 Ruff (0.15.0)
[warning] 120-120: Use explicit conversion flag
Replace with conversion flag
(RUF010)
🤖 Prompt for AI Agents
In
`@art-cluster/cloudfront/mirror/lambda_art-srv-enterprise-s3-get-index-html-gen.py`
around lines 119 - 121, The print statement uses an unnecessary str() inside an
f-string (print(f"Traversing dir {str(path_top_dir)}")) which violates RUF010;
update the f-string to use the !s conversion flag instead (e.g., f"Traversing
dir {path_top_dir!s}") so you remove str() and still stringify path_top_dir;
change the call where VERBOSE is checked (the block that prints when VERBOSE is
true) to use the !s conversion.
| if uri.startswith("/pockets/"): | ||
| # The username for pockets should be of the form '<pocketName>+<anonymized user id>' . Extract the pocket | ||
| # name. The user must only have access to the pocket specified in their username. | ||
| if username.index('+') > 0: | ||
| if username.index("+") > 0: | ||
| if not POCKET_SERVICE_ACCOUNTS: | ||
| POCKET_SERVICE_ACCOUNTS = get_secrets_manager_secret_dict( | ||
| 'art_srv_request_basic_auth/POCKET_SERVICE_ACCOUNTS' | ||
| "art_srv_request_basic_auth/POCKET_SERVICE_ACCOUNTS" | ||
| ) | ||
| pocket_name = username.split('+')[0] | ||
| if uri.startswith(f'/pockets/{pocket_name}/'): | ||
| pocket_name = username.split("+")[0] | ||
| if uri.startswith(f"/pockets/{pocket_name}/"): |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if file exists and read the relevant lines
FILE="art-cluster/cloudfront/mirror/lambda_art-srv-request-basic-auth.py"
if [ -f "$FILE" ]; then
echo "=== File exists, checking line range 589-598 ==="
sed -n '589,598p' "$FILE"
echo ""
echo "=== Searching for all index(\"+\") usage in file ==="
rg -n 'index\("\+"\)' "$FILE"
else
echo "File not found: $FILE"
# Try to find the file
find . -name "lambda_art-srv-request-basic-auth.py" 2>/dev/null
fiRepository: openshift-eng/art-tools
Length of output: 845
Avoid str.index() to prevent ValueError on usernames without "+".
The code at line 592 uses username.index("+") which raises ValueError if the character is missing, causing a 500 error instead of proper authorization failure. Replace with find() to return -1 gracefully (which fails the > 0 check without throwing).
Suggested fix
- if username.index("+") > 0:
+ if username.find("+") > 0:
if not POCKET_SERVICE_ACCOUNTS:
POCKET_SERVICE_ACCOUNTS = get_secrets_manager_secret_dict(
"art_srv_request_basic_auth/POCKET_SERVICE_ACCOUNTS"
)
- pocket_name = username.split("+")[0]
+ pocket_name = username.split("+", 1)[0]
if uri.startswith(f"/pockets/{pocket_name}/"):🤖 Prompt for AI Agents
In `@art-cluster/cloudfront/mirror/lambda_art-srv-request-basic-auth.py` around
lines 589 - 598, The authorization path handling pockets uses
username.index("+") which throws on missing "+"; change the check to use a safe
lookup (e.g., username.find("+") or `"+" in username`) so it returns -1/False
instead of raising; keep the surrounding logic that lazy-loads
POCKET_SERVICE_ACCOUNTS via get_secrets_manager_secret_dict and that extracts
pocket_name = username.split("+")[0] only when the separator exists and the
uri.startswith(f"/pockets/{pocket_name}/") check applies.
| if uri.startswith("/pockets/"): | ||
| # The username for pockets should be of the form '<pocketName>+<anonymized user id>' . Extract the pocket | ||
| # name. The user must only have access to the pocket specified in their username. | ||
| if username.index('+') > 0: | ||
| if username.index("+") > 0: | ||
| if not POCKET_SERVICE_ACCOUNTS: | ||
| POCKET_SERVICE_ACCOUNTS = get_secrets_manager_secret_dict( | ||
| 'art_srv_request_basic_auth/POCKET_SERVICE_ACCOUNTS' | ||
| "art_srv_request_basic_auth/POCKET_SERVICE_ACCOUNTS" | ||
| ) | ||
| pocket_name = username.split('+')[0] | ||
| if uri.startswith(f'/pockets/{pocket_name}/'): | ||
| pocket_name = username.split("+")[0] | ||
| if uri.startswith(f"/pockets/{pocket_name}/"): | ||
| if username in POCKET_SERVICE_ACCOUNTS: |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
git ls-files | grep -i "lambda_r2_art-srv-request-basic-auth.py"Repository: openshift-eng/art-tools
Length of output: 136
🏁 Script executed:
cat -n art-cluster/cloudfront/mirror/lambda_r2_art-srv-request-basic-auth.py | sed -n '130,170p'Repository: openshift-eng/art-tools
Length of output: 2534
🏁 Script executed:
# Also check file size to understand the full context
wc -l art-cluster/cloudfront/mirror/lambda_r2_art-srv-request-basic-auth.pyRepository: openshift-eng/art-tools
Length of output: 140
🏁 Script executed:
# Look for the unauthorized() function and any error handling patterns
rg -n "def unauthorized|return unauthorized" art-cluster/cloudfront/mirror/lambda_r2_art-srv-request-basic-auth.pyRepository: openshift-eng/art-tools
Length of output: 279
Handle ValueError for pocket usernames without "+".
username.index("+") raises when "+" is absent, causing a 500 error for /pockets/ requests with invalid credentials instead of proper 401 rejection. Use find() for safe substring detection:
Suggested fix
- if username.index("+") > 0:
+ plus_pos = username.find("+")
+ if plus_pos > 0:
if not POCKET_SERVICE_ACCOUNTS:
POCKET_SERVICE_ACCOUNTS = get_secrets_manager_secret_dict(
"art_srv_request_basic_auth/POCKET_SERVICE_ACCOUNTS"
)
- pocket_name = username.split("+")[0]
+ pocket_name = username[:plus_pos]
if uri.startswith(f"/pockets/{pocket_name}/"):
if username in POCKET_SERVICE_ACCOUNTS:
if hmac.compare_digest(password, POCKET_SERVICE_ACCOUNTS[username]):
authorized = True
+ else:
+ return unauthorized()🤖 Prompt for AI Agents
In `@art-cluster/cloudfront/mirror/lambda_r2_art-srv-request-basic-auth.py` around
lines 147 - 157, Change the unsafe username.index("+") check to a safe existence
check so missing "+" doesn’t raise ValueError and cause 500s: in the code block
that handles pocket access (symbols: POCKET_SERVICE_ACCOUNTS,
get_secrets_manager_secret_dict, pocket_name, and the
uri.startswith(f"/pockets/{pocket_name}/") branch) replace the if
username.index("+") > 0 check with a safe check such as if "+" in username and
username.find("+") > 0 (or username.split("+",1) length check), and ensure that
when the "+" is absent the code falls through to return a 401 rather than
raising an exception.
| # Same for robots.txt so bots don't need to follow redirect. | ||
| pass | ||
| elif request.get('method', None).lower() != "get": | ||
| elif request.get("method", None).lower() != "get": |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n art-cluster/cloudfront/mirror/lambda_r2_art-srv-request-basic-auth.py | sed -n '170,190p'Repository: openshift-eng/art-tools
Length of output: 1412
🏁 Script executed:
cat -n art-cluster/cloudfront/mirror/lambda_r2_art-srv-request-basic-auth.py | head -50Repository: openshift-eng/art-tools
Length of output: 1622
🏁 Script executed:
cat -n art-cluster/cloudfront/mirror/lambda_r2_art-srv-request-basic-auth.py | sed -n '100,180p'Repository: openshift-eng/art-tools
Length of output: 4729
🏁 Script executed:
cat -n art-cluster/cloudfront/mirror/lambda_r2_art-srv-request-basic-auth.py | sed -n '50,110p'Repository: openshift-eng/art-tools
Length of output: 2547
🏁 Script executed:
cat -n art-cluster/cloudfront/mirror/lambda_r2_art-srv-request-basic-auth.py | tail -20Repository: openshift-eng/art-tools
Length of output: 1429
🏁 Script executed:
rg "method" art-cluster/cloudfront/mirror/lambda_r2_art-srv-request-basic-auth.py -nRepository: openshift-eng/art-tools
Length of output: 125
🏁 Script executed:
find . -type f \( -name "*.py" -o -name "*.md" -o -name "*.txt" -o -name "*.json" \) | xargs grep -l "method" | head -10Repository: openshift-eng/art-tools
Length of output: 562
🏁 Script executed:
cat art-cluster/cloudfront/mirror/README.mdRepository: openshift-eng/art-tools
Length of output: 7367
🌐 Web query:
AWS CloudFront Lambda@Edge viewer request event structure method field
💡 Result:
In a Lambda@Edge viewer-request trigger, the HTTP method is in the request object:
- Path:
event.Records[0].cf.request.method - Type/values: string like
"GET","POST", etc. (docs.aws.amazon.com)
Example (trimmed):
{
"Records": [
{
"cf": {
"request": {
"method": "GET",
"uri": "/",
"querystring": ""
}
}
}
]
}
``` ([docs.aws.amazon.com](https://docs.aws.amazon.com/AmazonCloudFront/latest/DeveloperGuide/lambda-event-structure.html?utm_source=openai))
Citations:
- 1: https://docs.aws.amazon.com/AmazonCloudFront/latest/DeveloperGuide/lambda-event-structure.html?utm_source=openai
- 2: https://docs.aws.amazon.com/AmazonCloudFront/latest/DeveloperGuide/lambda-event-structure.html?utm_source=openai
---
</details>
**Guard against missing method values.**
`request.get("method", None).lower()` will throw an `AttributeError` if `method` is `None`. While AWS CloudFront guarantees the `method` field is always present in the request object, the current defensive pattern is broken. Consider using a safe default:
<details>
<summary>Suggested fix</summary>
```diff
- elif request.get("method", None).lower() != "get":
+ elif (request.get("method") or "").lower() != "get":🤖 Prompt for AI Agents
In `@art-cluster/cloudfront/mirror/lambda_r2_art-srv-request-basic-auth.py` at
line 179, The conditional uses request.get("method", None).lower() which will
raise AttributeError if method is None; change the check in the elif that
references request.get("method", None).lower() to first obtain the method value
and guard it (either use a safe empty-string default or an explicit None check)
so lower() is only called on a string—e.g., read method = request.get("method")
and then test method is not None and method.lower() != "get" (or use (method or
"") before .lower()) to avoid the exception.
| succeeded_condition = pipelinerun_info.find_condition("Succeeded") | ||
| outcome = KonfluxBuildOutcome.extract_from_pipelinerun_succeeded_condition(succeeded_condition) | ||
|
|
||
| # Even if the build succeeded, if the SLSA attestation cannot be retrieved, it is unreleasable. | ||
| if outcome is KonfluxBuildOutcome.SUCCESS: | ||
| results = pipelinerun_info.to_dict().get('status', {}).get('results', []) | ||
| image_pullspec = next((r['value'] for r in results if r['name'] == 'IMAGE_URL'), None) | ||
| image_digest = next((r['value'] for r in results if r['name'] == 'IMAGE_DIGEST'), None) | ||
| results = pipelinerun_info.to_dict().get("status", {}).get("results", []) | ||
| image_pullspec = next((r["value"] for r in results if r["name"] == "IMAGE_URL"), None) | ||
| image_digest = next((r["value"] for r in results if r["name"] == "IMAGE_DIGEST"), None) | ||
|
|
||
| record["image_pullspec"] = f"{image_pullspec.split(':')[0]}@{image_digest}" | ||
|
|
||
| image_tag = image_pullspec.split(':')[-1] | ||
| image_tag = image_pullspec.split(":")[-1] | ||
| record["image_tag"] = image_tag |
There was a problem hiding this comment.
Guard against missing IMAGE_URL/IMAGE_DIGEST before split.
Line 224-Line 231 uses image_pullspec.split(...) even if results are missing; that will blow up and skip DB updates. Consider explicitly validating and flipping the outcome to failure (or raising a clearer error) before dereferencing.
🛠️ Suggested guard
if outcome is KonfluxBuildOutcome.SUCCESS:
results = pipelinerun_info.to_dict().get("status", {}).get("results", [])
image_pullspec = next((r["value"] for r in results if r["name"] == "IMAGE_URL"), None)
image_digest = next((r["value"] for r in results if r["name"] == "IMAGE_DIGEST"), None)
- record["image_pullspec"] = f"{image_pullspec.split(':')[0]}@{image_digest}"
-
- image_tag = image_pullspec.split(":")[-1]
- record["image_tag"] = image_tag
-
- # Validate SLSA attestation and source image signature
- try:
- await self._validate_build_attestation_and_signature(image_pullspec, metadata.distgit_key)
- except Exception as e:
- logger.error(
- f"Failed to get SLA attestation / source signature from konflux for image {image_pullspec}, marking build as {KonfluxBuildOutcome.FAILURE}. Error: {e}"
- )
- outcome = KonfluxBuildOutcome.FAILURE
+ if not (image_pullspec and image_digest):
+ logger.error(
+ "Missing IMAGE_URL/IMAGE_DIGEST in PipelineRun results for %s; marking build as %s",
+ pipelinerun_name,
+ KonfluxBuildOutcome.FAILURE,
+ )
+ outcome = KonfluxBuildOutcome.FAILURE
+ else:
+ record["image_pullspec"] = f"{image_pullspec.split(':')[0]}@{image_digest}"
+ image_tag = image_pullspec.split(":")[-1]
+ record["image_tag"] = image_tag
+
+ # Validate SLSA attestation and source image signature
+ try:
+ await self._validate_build_attestation_and_signature(image_pullspec, metadata.distgit_key)
+ except Exception as e:
+ logger.error(
+ f"Failed to get SLA attestation / source signature from konflux for image {image_pullspec}, marking build as {KonfluxBuildOutcome.FAILURE}. Error: {e}"
+ )
+ outcome = KonfluxBuildOutcome.FAILURE🤖 Prompt for AI Agents
In `@doozer/doozerlib/backend/konflux_image_builder.py` around lines 219 - 231,
The code assumes IMAGE_URL and IMAGE_DIGEST exist and calls
image_pullspec.split(...) which will raise if missing; update the block that
handles KonfluxBuildOutcome.SUCCESS in konflux_image_builder to validate results
before splitting: fetch results via pipelinerun_info.to_dict().get("status",
{}).get("results", []) and check that IMAGE_URL and IMAGE_DIGEST values are
present and non-empty (e.g., variables image_pullspec and image_digest are not
None); if either is missing, flip the outcome to a non-releaseable state (e.g.,
set outcome = KonfluxBuildOutcome.FAILURE or mark unreleasable and log the
missing keys using pipelinerun_info and record) instead of performing split(),
and only compute record["image_pullspec"] and record["image_tag"] when
image_pullspec and image_digest are valid.
| res = requests.post( | ||
| f'{ERRATA_API_URL}/erratum/{advisory_id}/remove_build', | ||
| f"{ERRATA_API_URL}/erratum/{advisory_id}/remove_build", | ||
| verify=ssl.get_default_verify_paths().openssl_cafile, | ||
| auth=auth, | ||
| json=remove_nvr_payload, | ||
| ) |
There was a problem hiding this comment.
Add timeouts to Errata API POSTs to avoid indefinite hangs.
Both POST calls to Errata lack a timeout, which can block the CLI indefinitely on network stalls. Consider adding a bounded timeout (or plumb a configurable value) to keep failure modes predictable.
🛠️ Proposed fix (example timeout)
res = requests.post(
f"{ERRATA_API_URL}/erratum/{advisory_id}/remove_build",
verify=ssl.get_default_verify_paths().openssl_cafile,
auth=auth,
json=remove_nvr_payload,
+ timeout=30,
)
@@
res = requests.post(
f"{ERRATA_API_URL}/erratum/{advisory_id}/add_builds",
verify=ssl.get_default_verify_paths().openssl_cafile,
auth=HTTPKerberosAuth(),
json=add_builds_payload,
+ timeout=30,
)Also applies to: 187-192
🧰 Tools
🪛 Ruff (0.15.0)
[error] 159-159: Probable use of requests call without timeout
(S113)
🤖 Prompt for AI Agents
In `@doozer/doozerlib/cli/config_plashet.py` around lines 159 - 164, The POST
requests to the Errata API in config_plashet.py (the requests.post call that
assigns to res when calling /erratum/{advisory_id}/remove_build and the other
POST at lines ~187-192) lack a timeout and can hang indefinitely; update both
requests.post invocations to include a bounded timeout argument (e.g.,
timeout=10 or, better, a configurable timeout value read from existing config or
an added CLI option) so the calls fail fast on network stalls and handle timeout
exceptions accordingly in the surrounding logic (keep the verify, auth, and json
args intact and add timeout to the same call sites).
| "--plr-template", | ||
| required=False, | ||
| default=constants.KONFLUX_DEFAULT_FBC_BUILD_PLR_TEMPLATE_URL, | ||
| help='Use a custom PipelineRun template to build the FBC fragement. Overrides the default template from openshift-priv/art-konflux-template', | ||
| help="Use a custom PipelineRun template to build the FBC fragement. Overrides the default template from openshift-priv/art-konflux-template", |
There was a problem hiding this comment.
Fix typo in help text (“fragement” → “fragment”).
User-facing CLI help should not contain spelling errors.
✏️ Proposed fix
- help="Use a custom PipelineRun template to build the FBC fragement. Overrides the default template from openshift-priv/art-konflux-template",
+ help="Use a custom PipelineRun template to build the FBC fragment. Overrides the default template from openshift-priv/art-konflux-template",Also applies to: 809-813
🤖 Prompt for AI Agents
In `@doozer/doozerlib/cli/fbc.py` around lines 372 - 375, The help text for the
CLI option "--plr-template" contains a typo "fragement"; update the help string
to use "fragment" wherever this option is defined (the add_argument call that
sets default=constants.KONFLUX_DEFAULT_FBC_BUILD_PLR_TEMPLATE_URL and its
duplicate further down around the second definition). Locate the add_argument
for "--plr-template" and replace "build the FBC fragement" with "build the FBC
fragment" in both occurrences so the user-facing CLI help is corrected.
| class ConcernCode(Enum): | ||
| NEVER_BUILT = 'NEVER_BUILT' | ||
| LATEST_ATTEMPT_FAILED = 'LATEST_ATTEMPT_FAILED' | ||
| FAILING_AT_LEAST_FOR = 'FAILING_AT_LEAST_FOR' | ||
| LATEST_BUILD_SUCCEEDED = 'LATEST_BUILT_SUCCEEDED' | ||
| NEVER_BUILT = "NEVER_BUILT" | ||
| LATEST_ATTEMPT_FAILED = "LATEST_ATTEMPT_FAILED" | ||
| FAILING_AT_LEAST_FOR = "FAILING_AT_LEAST_FOR" | ||
| LATEST_BUILD_SUCCEEDED = "LATEST_BUILT_SUCCEEDED" |
There was a problem hiding this comment.
Enum value typo breaks external consumers
Line 24 changes the value to "LATEST_BUILT_SUCCEEDED" (missing “BUILD”). This will alter the serialized concern codes and can break downstream consumers expecting "LATEST_BUILD_SUCCEEDED". If this wasn’t intentional, revert the value.
🩹 Proposed fix
- LATEST_BUILD_SUCCEEDED = "LATEST_BUILT_SUCCEEDED"
+ LATEST_BUILD_SUCCEEDED = "LATEST_BUILD_SUCCEEDED"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| class ConcernCode(Enum): | |
| NEVER_BUILT = 'NEVER_BUILT' | |
| LATEST_ATTEMPT_FAILED = 'LATEST_ATTEMPT_FAILED' | |
| FAILING_AT_LEAST_FOR = 'FAILING_AT_LEAST_FOR' | |
| LATEST_BUILD_SUCCEEDED = 'LATEST_BUILT_SUCCEEDED' | |
| NEVER_BUILT = "NEVER_BUILT" | |
| LATEST_ATTEMPT_FAILED = "LATEST_ATTEMPT_FAILED" | |
| FAILING_AT_LEAST_FOR = "FAILING_AT_LEAST_FOR" | |
| LATEST_BUILD_SUCCEEDED = "LATEST_BUILT_SUCCEEDED" | |
| class ConcernCode(Enum): | |
| NEVER_BUILT = "NEVER_BUILT" | |
| LATEST_ATTEMPT_FAILED = "LATEST_ATTEMPT_FAILED" | |
| FAILING_AT_LEAST_FOR = "FAILING_AT_LEAST_FOR" | |
| LATEST_BUILD_SUCCEEDED = "LATEST_BUILD_SUCCEEDED" |
🤖 Prompt for AI Agents
In `@doozer/doozerlib/cli/images_health.py` around lines 20 - 24, The ConcernCode
enum member LATEST_BUILD_SUCCEEDED has an incorrect string value
"LATEST_BUILT_SUCCEEDED" which will change serialized concern codes; update the
value of ConcernCode.LATEST_BUILD_SUCCEEDED to the correct string
"LATEST_BUILD_SUCCEEDED" so external consumers keep the expected serialized
token and tests/clients remain compatible.
| temp_dir = tempfile.mkdtemp() | ||
| try: | ||
| oc.invoke('image', cmd_args=['extract', f'--path=/etc/centos-release:{temp_dir}', latest_pullspec]) | ||
| oc.invoke("image", cmd_args=["extract", f"--path=/etc/centos-release:{temp_dir}", latest_pullspec]) | ||
| if tag not in payload_tag_names: | ||
| detail = 'not in ART built OKD tags' | ||
| detail = "not in ART built OKD tags" | ||
|
|
||
| if os.path.exists(f'{temp_dir}/centos-release'): | ||
| if os.path.exists(f"{temp_dir}/centos-release"): | ||
| scos_status = "Yes" | ||
| else: | ||
| if tag not in payload_tag_names: | ||
| scos_status = "N/A?" | ||
|
|
||
| except: | ||
| detail = 'error checking image' | ||
| detail = "error checking image" | ||
| shutil.rmtree(temp_dir) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, check if the file exists and view the relevant lines
if [ -f "doozer/doozerlib/cli/images_okd.py" ]; then
echo "=== Lines 713-727 from images_okd.py ==="
sed -n '713,727p' doozer/doozerlib/cli/images_okd.py | cat -n
else
echo "File not found at expected location, searching for it..."
find . -name "images_okd.py" -type f
fiRepository: openshift-eng/art-tools
Length of output: 876
🏁 Script executed:
# Check broader context to understand the function and exception handling
if [ -f "doozer/doozerlib/cli/images_okd.py" ]; then
echo "=== Broader context (lines 700-735) ==="
sed -n '700,735p' doozer/doozerlib/cli/images_okd.py | cat -n
fiRepository: openshift-eng/art-tools
Length of output: 1829
Replace bare except with except Exception and use finally for cleanup.
The bare except catches KeyboardInterrupt and SystemExit, masking program interrupts. Additionally, moving shutil.rmtree(temp_dir) into a finally block ensures cleanup runs regardless of exception handling outcome while preserving exception context.
Suggested adjustment
temp_dir = tempfile.mkdtemp()
try:
oc.invoke("image", cmd_args=["extract", f"--path=/etc/centos-release:{temp_dir}", latest_pullspec])
if tag not in payload_tag_names:
detail = "not in ART built OKD tags"
if os.path.exists(f"{temp_dir}/centos-release"):
scos_status = "Yes"
else:
if tag not in payload_tag_names:
scos_status = "N/A?"
- except:
+ except Exception as e:
- detail = "error checking image"
- shutil.rmtree(temp_dir)
+ detail = f"error checking image: {e}"
+ finally:
+ shutil.rmtree(temp_dir)🧰 Tools
🪛 Ruff (0.15.0)
[error] 725-725: Do not use bare except
(E722)
🤖 Prompt for AI Agents
In `@doozer/doozerlib/cli/images_okd.py` around lines 713 - 727, Replace the bare
except in the block that invokes oc.invoke (using temp_dir, latest_pullspec,
tag, payload_tag_names) with an explicit "except Exception as e" so
system-exiting signals aren't swallowed, capture the exception in a variable (e)
and set detail accordingly, and move the cleanup call shutil.rmtree(temp_dir)
into a finally block so temp_dir is always removed after the try/except; ensure
scos_status and detail logic remains unchanged inside the try/except/finally
structure.
| def calc_parent_digest(parent_images): | ||
| m = hashlib.md5() | ||
| m.update(';'.join(parent_images).encode('utf-8')) | ||
| m.update(";".join(parent_images).encode("utf-8")) | ||
| return m.hexdigest() | ||
|
|
||
|
|
||
| def extract_parent_digest(dockerfile_path): | ||
| with dockerfile_path.open(mode='r') as handle: | ||
| with dockerfile_path.open(mode="r") as handle: | ||
| dfp = DockerfileParser(cache_content=True, fileobj=io.BytesIO()) | ||
| dfp.content = handle.read() | ||
| return calc_parent_digest(dfp.parent_images), dfp.parent_images | ||
|
|
||
|
|
||
| def compute_dockerfile_digest(dockerfile_path): | ||
| with dockerfile_path.open(mode='r') as handle: | ||
| with dockerfile_path.open(mode="r") as handle: | ||
| # Read in and standardize linefeed | ||
| content = '\n'.join(handle.read().splitlines()) | ||
| content = "\n".join(handle.read().splitlines()) | ||
| m = hashlib.md5() | ||
| m.update(content.encode('utf-8')) | ||
| m.update(content.encode("utf-8")) | ||
| return m.hexdigest() |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find and check Ruff configuration files
echo "=== Checking for Ruff configuration ==="
find . -maxdepth 3 -type f \( -name "pyproject.toml" -o -name "ruff.toml" -o -name ".ruff.toml" \) -exec echo "Found: {}" \;
# Check the file in question
echo -e "\n=== Target file content (lines 720-750) ==="
if [ -f "doozer/doozerlib/cli/images_streams.py" ]; then
sed -n '720,750p' "doozer/doozerlib/cli/images_streams.py" | cat -n
else
echo "File not found at expected path"
# Try to find it
find . -name "images_streams.py" -type f
fiRepository: openshift-eng/art-tools
Length of output: 1381
🏁 Script executed:
#!/bin/bash
# Check Ruff configuration for S324 specifically
echo "=== Ruff config with S324 or select rules ==="
if [ -f "pyproject.toml" ]; then
rg -A 10 -B 2 "\[tool\.ruff" "pyproject.toml" | head -50
fi
echo -e "\n=== MD5 usage in the codebase ==="
rg -n "hashlib\.md5" --type=pyRepository: openshift-eng/art-tools
Length of output: 1510
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Full Ruff lint select list ==="
rg -A 30 "select = \[" pyproject.toml
echo -e "\n=== Check ruff.toml content ==="
cat ruff.toml 2>/dev/null || echo "ruff.toml not found or empty"Repository: openshift-eng/art-tools
Length of output: 1658
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Search for usage of digest functions ==="
rg -n "calc_parent_digest|extract_parent_digest|compute_dockerfile_digest" --type=py -B 2 -A 2
echo -e "\n=== Check if these digests are stored/compared (non-security use) ==="
rg -n "parent_digest|dockerfile_digest" --type=py | head -30Repository: openshift-eng/art-tools
Length of output: 4675
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Check which Ruff config takes precedence ==="
# Typically ruff.toml takes precedence over pyproject.toml
# Let's also check if there's any documentation reference
echo "File existence and timestamps:"
ls -l ruff.toml pyproject.toml 2>/dev/null || echo "Check files individually"
echo -e "\n=== Verify no noqa comments exist on the MD5 lines ==="
sed -n '727,730p' doozer/doozerlib/cli/images_streams.py
echo "---"
sed -n '740,747p' doozer/doozerlib/cli/images_streams.pyRepository: openshift-eng/art-tools
Length of output: 765
Address Ruff S324: switch to SHA-256 or add explicit noqa comment.
Lines 728 and 744 use hashlib.md5(), which is flagged by Ruff's S324 rule now enabled in ruff.toml. Since these digests are used only for change detection (equality comparisons at lines 1243, 1394, 1410, 1507), switch to SHA-256 or add an explicit # noqa: S324 comment.
Suggested fix (switch to SHA-256)
def calc_parent_digest(parent_images):
- m = hashlib.md5()
+ m = hashlib.sha256()
m.update(";".join(parent_images).encode("utf-8"))
return m.hexdigest()
def compute_dockerfile_digest(dockerfile_path):
with dockerfile_path.open(mode="r") as handle:
# Read in and standardize linefeed
content = "\n".join(handle.read().splitlines())
- m = hashlib.md5()
+ m = hashlib.sha256()
m.update(content.encode("utf-8"))
return m.hexdigest()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def calc_parent_digest(parent_images): | |
| m = hashlib.md5() | |
| m.update(';'.join(parent_images).encode('utf-8')) | |
| m.update(";".join(parent_images).encode("utf-8")) | |
| return m.hexdigest() | |
| def extract_parent_digest(dockerfile_path): | |
| with dockerfile_path.open(mode='r') as handle: | |
| with dockerfile_path.open(mode="r") as handle: | |
| dfp = DockerfileParser(cache_content=True, fileobj=io.BytesIO()) | |
| dfp.content = handle.read() | |
| return calc_parent_digest(dfp.parent_images), dfp.parent_images | |
| def compute_dockerfile_digest(dockerfile_path): | |
| with dockerfile_path.open(mode='r') as handle: | |
| with dockerfile_path.open(mode="r") as handle: | |
| # Read in and standardize linefeed | |
| content = '\n'.join(handle.read().splitlines()) | |
| content = "\n".join(handle.read().splitlines()) | |
| m = hashlib.md5() | |
| m.update(content.encode('utf-8')) | |
| m.update(content.encode("utf-8")) | |
| return m.hexdigest() | |
| def calc_parent_digest(parent_images): | |
| m = hashlib.sha256() | |
| m.update(";".join(parent_images).encode("utf-8")) | |
| return m.hexdigest() | |
| def extract_parent_digest(dockerfile_path): | |
| with dockerfile_path.open(mode="r") as handle: | |
| dfp = DockerfileParser(cache_content=True, fileobj=io.BytesIO()) | |
| dfp.content = handle.read() | |
| return calc_parent_digest(dfp.parent_images), dfp.parent_images | |
| def compute_dockerfile_digest(dockerfile_path): | |
| with dockerfile_path.open(mode="r") as handle: | |
| # Read in and standardize linefeed | |
| content = "\n".join(handle.read().splitlines()) | |
| m = hashlib.sha256() | |
| m.update(content.encode("utf-8")) | |
| return m.hexdigest() |
🧰 Tools
🪛 Ruff (0.15.0)
[error] 728-728: Probable use of insecure hash functions in hashlib: md5
(S324)
[error] 744-744: Probable use of insecure hash functions in hashlib: md5
(S324)
🤖 Prompt for AI Agents
In `@doozer/doozerlib/cli/images_streams.py` around lines 727 - 746, The code uses
insecure hashlib.md5 in calc_parent_digest and compute_dockerfile_digest (and
indirectly via extract_parent_digest); replace hashlib.md5() with
hashlib.sha256() in both calc_parent_digest and compute_dockerfile_digest so
digests use SHA-256 for change detection, and ensure any places that compare
these digests (callers of extract_parent_digest, calc_parent_digest,
compute_dockerfile_digest) continue to treat them as opaque hex strings (no
other logic depends on MD5 length). Alternatively, if you intentionally want to
keep MD5, add an explicit "# noqa: S324" on the hashlib.md5() lines, but
preferred fix is switching to hashlib.sha256().
2 participants
the core change is in the pyproject.toml, where I set
I chose double quotes cause I prefer them and also ruff suggest to use them in their example configuration, while PEP8 simply says to choose one and stay consistent 😃
for folks using vscode, I've this very handy formatOnSave in my settings.json:
N.B. all these changes have been done simply running a
make lintcommand