test#2272
Conversation
PR Reviewer Guide 🔍(Review updated until commit 1cd91b5)Warning
Here are some key observations to aid the review process:
|
PR Reviewer Guide 🔍Warning
Here are some key observations to aid the review process:
|
PR Code Suggestions ✨Warning
Inline suggestions were posted as code suggestions. |
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
| - name: Assign reviewers using shared action | ||
| uses: konflux-ci/release-service-automations/pr-assigner@5da461efb1b29e523d01989ee09758e85099bda8 # main | ||
| uses: seanconroy2021/release-service-automations/pr-assigner@auto-pr |
There was a problem hiding this comment.
Suggestion: Pin the GitHub Action to a specific commit SHA instead of a mutable branch. [security, importance: 8]
| - name: Assign reviewers using shared action | |
| uses: konflux-ci/release-service-automations/pr-assigner@5da461efb1b29e523d01989ee09758e85099bda8 # main | |
| uses: seanconroy2021/release-service-automations/pr-assigner@auto-pr | |
| - name: Assign reviewers using shared action | |
| uses: seanconroy2021/release-service-automations/pr-assigner@<COMMIT_SHA> |
PR Code Suggestions ✨Warning
Inline suggestions were posted as code suggestions. |
![qodo-app-for-konflux-ci[bot]](https://avatars.githubusercontent.com/u/155583288?s=60&v=4){kind=small}
| - name: Assign reviewers using shared action | ||
| uses: konflux-ci/release-service-automations/pr-assigner@5da461efb1b29e523d01989ee09758e85099bda8 # main | ||
| uses: seanconroy2021/release-service-automations/pr-assigner@auto-pr |
There was a problem hiding this comment.
Suggestion: Pin the GitHub Action to an immutable commit SHA instead of a movable branch like @auto-pr. [security, importance: 9]
| - name: Assign reviewers using shared action | |
| uses: konflux-ci/release-service-automations/pr-assigner@5da461efb1b29e523d01989ee09758e85099bda8 # main | |
| uses: seanconroy2021/release-service-automations/pr-assigner@auto-pr | |
| - name: Assign reviewers using shared action | |
| uses: seanconroy2021/release-service-automations/pr-assigner@<COMMIT_SHA> |
PR Code Suggestions ✨Warning
Inline suggestions were posted as code suggestions. |
There was a problem hiding this comment.
High-level Suggestion
Restore the original canonical GitHub Action reference and its secure commit SHA. [High-level, importance: 9]
Solution Walkthrough:
Before:
jobs:
assign-reviewers:
runs-on: ubuntu-latest
steps:
- name: Assign reviewers using shared action
uses: seanconroy2021/release-service-automations/pr-assigner@auto-pr
with:
event-type: ${{ github.event.action }}
After:
jobs:
assign-reviewers:
runs-on: ubuntu-latest
steps:
- name: Assign reviewers using shared action
uses: konflux-ci/release-service-automations/pr-assigner@5da461efb1b29e523d01989ee09758e85099bda8
with:
event-type: ${{ github.event.action }}
| def extract_pipeline_tasks(pipeline_yaml: str) -> Dict[str, str]: | ||
| """Parse a pipeline definition and return a map of task name to pathInRepo. | ||
|
|
||
| Args: | ||
| pipeline_yaml: Raw YAML content of the pipeline file. | ||
|
|
||
| Returns: | ||
| Dict mapping each pipeline task name to its pathInRepo value. | ||
| Tasks without a pathInRepo get an empty string. | ||
| """ | ||
| doc = yaml.safe_load(pipeline_yaml) | ||
| tasks: Dict[str, str] = {} | ||
| task_blocks = doc.get("spec", {}).get("tasks", []) | ||
| finally_blocks = doc.get("spec", {}).get("finally", []) | ||
|
|
||
| for block in task_blocks + finally_blocks: | ||
| name = block.get("name", "") | ||
| ref = block.get("taskRef", {}) | ||
| if not isinstance(ref, dict): | ||
| tasks[name] = "" | ||
| continue | ||
|
|
||
| params = {p["name"]: p["value"] for p in ref.get("params", [])} |
There was a problem hiding this comment.
Suggestion: Suggest using or fallbacks when parsing YAML dictionaries to prevent errors from null values. [possible issue, importance: 7]
| def extract_pipeline_tasks(pipeline_yaml: str) -> Dict[str, str]: | |
| """Parse a pipeline definition and return a map of task name to pathInRepo. | |
| Args: | |
| pipeline_yaml: Raw YAML content of the pipeline file. | |
| Returns: | |
| Dict mapping each pipeline task name to its pathInRepo value. | |
| Tasks without a pathInRepo get an empty string. | |
| """ | |
| doc = yaml.safe_load(pipeline_yaml) | |
| tasks: Dict[str, str] = {} | |
| task_blocks = doc.get("spec", {}).get("tasks", []) | |
| finally_blocks = doc.get("spec", {}).get("finally", []) | |
| for block in task_blocks + finally_blocks: | |
| name = block.get("name", "") | |
| ref = block.get("taskRef", {}) | |
| if not isinstance(ref, dict): | |
| tasks[name] = "" | |
| continue | |
| params = {p["name"]: p["value"] for p in ref.get("params", [])} | |
| def extract_pipeline_tasks(pipeline_yaml: str) -> Dict[str, str]: | |
| """Parse a pipeline definition and return a map of task name to pathInRepo. | |
| ... | |
| """ | |
| doc = yaml.safe_load(pipeline_yaml) or {} | |
| tasks: Dict[str, str] = {} | |
| spec = doc.get("spec") or {} | |
| task_blocks = spec.get("tasks") or [] | |
| finally_blocks = spec.get("finally") or [] | |
| for block in task_blocks + finally_blocks: | |
| name = block.get("name", "") | |
| ref = block.get("taskRef") or {} | |
| if not isinstance(ref, dict): | |
| tasks[name] = "" | |
| continue | |
| params = {p["name"]: p["value"] for p in (ref.get("params") or [])} |
| def extract_rpa_fields(rpa: dict) -> Tuple[str, str, List[dict]]: | ||
| """Extract the revision, pipeline path, and taskRunSpecs from an RPA. | ||
|
|
||
| Args: | ||
| rpa: Parsed RPA document. | ||
|
|
||
| Returns: | ||
| Tuple of (revision, pathInRepo, taskRunSpecs list). | ||
| """ | ||
| pipeline = rpa.get("spec", {}).get("pipeline", {}) | ||
| ref_params = pipeline.get("pipelineRef", {}).get("params", []) | ||
| params = {p["name"]: p["value"] for p in ref_params} | ||
|
|
||
| revision = params.get("revision", "") | ||
| path_in_repo = params.get("pathInRepo", "") | ||
| task_run_specs = pipeline.get("taskRunSpecs", []) |
There was a problem hiding this comment.
Suggestion: Recommend boolean or evaluations to safely handle null mappings in YAML and avoid exceptions. [possible issue, importance: 7]
| def extract_rpa_fields(rpa: dict) -> Tuple[str, str, List[dict]]: | |
| """Extract the revision, pipeline path, and taskRunSpecs from an RPA. | |
| Args: | |
| rpa: Parsed RPA document. | |
| Returns: | |
| Tuple of (revision, pathInRepo, taskRunSpecs list). | |
| """ | |
| pipeline = rpa.get("spec", {}).get("pipeline", {}) | |
| ref_params = pipeline.get("pipelineRef", {}).get("params", []) | |
| params = {p["name"]: p["value"] for p in ref_params} | |
| revision = params.get("revision", "") | |
| path_in_repo = params.get("pathInRepo", "") | |
| task_run_specs = pipeline.get("taskRunSpecs", []) | |
| def extract_rpa_fields(rpa: dict) -> Tuple[str, str, List[dict]]: | |
| """Extract the revision, pipeline path, and taskRunSpecs from an RPA. | |
| ... | |
| """ | |
| spec = rpa.get("spec") or {} | |
| pipeline = spec.get("pipeline") or {} | |
| pipeline_ref = pipeline.get("pipelineRef") or {} | |
| ref_params = pipeline_ref.get("params") or [] | |
| params = {p["name"]: p["value"] for p in ref_params} | |
| revision = params.get("revision", "") | |
| path_in_repo = params.get("pathInRepo", "") | |
| task_run_specs = pipeline.get("taskRunSpecs") or [] |
| LOGGER.error("Could not extract revision or pathInRepo from the RPA") | ||
| return 1 | ||
|
|
||
| has_template_vars = "${" in revision or "${" in pipeline_path |
There was a problem hiding this comment.
Suggestion: Suggest expanding template variable detection to include the $() pattern. [possible issue, importance: 6]
| has_template_vars = "${" in revision or "${" in pipeline_path | |
| has_template_vars = any(token in revision for token in ("${", "$(")) \ | |
| or any(token in pipeline_path for token in ("${", "$(")) |
| else: | ||
| LOGGER.info(f"Validation passed: {warnings} warning(s)") | ||
|
|
||
| return 1 if errors else 0 |
There was a problem hiding this comment.
Suggestion: Recommend removing the temporary clone_dir before successful script exit to avoid leaking directories. [general, importance: 4]
| return 1 if errors else 0 | |
| code = 1 if errors else 0 | |
| if clone_dir and not local: | |
| shutil.rmtree(clone_dir, ignore_errors=True) | |
| return code |
| - name: Assign reviewers using shared action | ||
| uses: konflux-ci/release-service-automations/pr-assigner@5da461efb1b29e523d01989ee09758e85099bda8 # main | ||
| uses: seanconroy2021/release-service-automations/pr-assigner@auto-pr | ||
| with: | ||
| event-type: ${{ github.event.action }} | ||
| pr-number: ${{ github.event.pull_request.number }} | ||
| removed-assignee: ${{ github.event.action == 'unassigned' && github.event.assignee.login || '' }} | ||
| github-token: ${{ secrets.PR_ASSIGNER_PAT_TOKEN }} | ||
| slack-webhook: ${{ secrets.PR_ASSIGNER_SLACK_WEBHOOK }} | ||
| pto-calendar-url: ${{ secrets.PTO_CALENDAR_URL }} |
There was a problem hiding this comment.
Suggestion: Add an in-file comment explaining why the action source was changed and what the new PTO_CALENDAR_URL secret is expected to contain/affect, to avoid surprising CI behavior and configuration drift. [organization best practice, importance: 6]
| - name: Assign reviewers using shared action | |
| uses: konflux-ci/release-service-automations/pr-assigner@5da461efb1b29e523d01989ee09758e85099bda8 # main | |
| uses: seanconroy2021/release-service-automations/pr-assigner@auto-pr | |
| with: | |
| event-type: ${{ github.event.action }} | |
| pr-number: ${{ github.event.pull_request.number }} | |
| removed-assignee: ${{ github.event.action == 'unassigned' && github.event.assignee.login || '' }} | |
| github-token: ${{ secrets.PR_ASSIGNER_PAT_TOKEN }} | |
| slack-webhook: ${{ secrets.PR_ASSIGNER_SLACK_WEBHOOK }} | |
| pto-calendar-url: ${{ secrets.PTO_CALENDAR_URL }} | |
| - name: Assign reviewers using shared action | |
| # NOTE: This workflow uses a custom PR assigner action fork; update rationale/owner here if changed. | |
| # Requires `PTO_CALENDAR_URL` secret (calendar feed used to avoid assigning reviewers who are OOO). | |
| uses: seanconroy2021/release-service-automations/pr-assigner@auto-pr | |
| with: | |
| event-type: ${{ github.event.action }} | |
| pr-number: ${{ github.event.pull_request.number }} | |
| removed-assignee: ${{ github.event.action == 'unassigned' && github.event.assignee.login || '' }} | |
| github-token: ${{ secrets.PR_ASSIGNER_PAT_TOKEN }} | |
| slack-webhook: ${{ secrets.PR_ASSIGNER_SLACK_WEBHOOK }} | |
| pto-calendar-url: ${{ secrets.PTO_CALENDAR_URL }} |
PR Code Suggestions ✨Warning
Inline suggestions were posted as code suggestions. |
| - name: Assign reviewers using shared action | ||
| uses: konflux-ci/release-service-automations/pr-assigner@5da461efb1b29e523d01989ee09758e85099bda8 # main | ||
| uses: seanconroy2021/release-service-automations/pr-assigner@auto-pr | ||
| with: |
There was a problem hiding this comment.
Suggestion: Revert the GitHub Action to use the official repository and a specific commit SHA. [security, importance: 9]
| - name: Assign reviewers using shared action | |
| uses: konflux-ci/release-service-automations/pr-assigner@5da461efb1b29e523d01989ee09758e85099bda8 # main | |
| uses: seanconroy2021/release-service-automations/pr-assigner@auto-pr | |
| with: | |
| - name: Assign reviewers using shared action | |
| uses: konflux-ci/release-service-automations/pr-assigner@5da461efb1b29e523d01989ee09758e85099bda8 # main | |
| with: |
| clone_dir = None | ||
| if not local: | ||
| LOGGER.info(f"Cloning {repo_url} at {revision}") | ||
| clone_dir = clone_repo(repo_url, revision) | ||
|
|
||
| pipeline_content = resolve_file(pipeline_path, revision, local, clone_dir) |
There was a problem hiding this comment.
Suggestion: Use atexit to ensure the temporary clone directory is reliably cleaned up. [possible issue, importance: 6]
| clone_dir = None | |
| if not local: | |
| LOGGER.info(f"Cloning {repo_url} at {revision}") | |
| clone_dir = clone_repo(repo_url, revision) | |
| pipeline_content = resolve_file(pipeline_path, revision, local, clone_dir) | |
| import atexit | |
| clone_dir = None | |
| if not local: | |
| LOGGER.info(f"Cloning {repo_url} at {revision}") | |
| clone_dir = clone_repo(repo_url, revision) | |
| atexit.register(shutil.rmtree, clone_dir, ignore_errors=True) | |
| pipeline_content = resolve_file(pipeline_path, revision, local, clone_dir) |
| doc = yaml.safe_load(pipeline_yaml) | ||
| tasks: Dict[str, str] = {} | ||
| task_blocks = doc.get("spec", {}).get("tasks", []) | ||
| finally_blocks = doc.get("spec", {}).get("finally", []) |
There was a problem hiding this comment.
Suggestion: Add fallback values to prevent crashes when parsing empty or null YAML documents. [general, importance: 8]
| doc = yaml.safe_load(pipeline_yaml) | |
| tasks: Dict[str, str] = {} | |
| task_blocks = doc.get("spec", {}).get("tasks", []) | |
| finally_blocks = doc.get("spec", {}).get("finally", []) | |
| doc = yaml.safe_load(pipeline_yaml) or {} | |
| tasks: Dict[str, str] = {} | |
| task_blocks = (doc.get("spec") or {}).get("tasks") or [] | |
| finally_blocks = (doc.get("spec") or {}).get("finally") or [] |
| LOGGER.error("Could not extract revision or pathInRepo from the RPA") | ||
| return 1 | ||
|
|
||
| has_template_vars = "${" in revision or "${" in pipeline_path |
There was a problem hiding this comment.
Suggestion: Expand template variable detection to include Tekton-style $() expressions. [possible issue, importance: 4]
| has_template_vars = "${" in revision or "${" in pipeline_path | |
| has_template_vars = any(token in revision for token in ("${", "$(")) \ | |
| or any(token in pipeline_path for token in ("${", "$(")) |
PR Code Suggestions ✨Warning
Inline suggestions were posted as code suggestions. |
| on: | ||
| pull_request_target: | ||
| pull_request: | ||
| types: [opened, ready_for_review, unassigned] |
There was a problem hiding this comment.
Suggestion: Revert to pull_request_target to ensure the workflow can access secrets for external contributor PRs. [possible issue, importance: 9]
| on: | |
| pull_request_target: | |
| pull_request: | |
| types: [opened, ready_for_review, unassigned] | |
| on: | |
| pull_request_target: | |
| types: [opened, ready_for_review, unassigned] | |
| ... | |
| jobs: | |
| assign-reviewers: | |
| if: ${{ github.event.pull_request.head.repo.full_name == github.repository }} | |
| runs-on: ubuntu-latest | |
| steps: | |
| - name: Assign reviewers using shared action | |
| uses: seanconroy2021/release-service-automations/pr-assigner@auto-pr | |
| with: | |
| event-type: ${{ github.event.action }} | |
| pr-number: ${{ github.event.pull_request.number }} | |
| removed-assignee: ${{ github.event.action == 'unassigned' && github.event.assignee.login || '' }} | |
| github-token: ${{ secrets.PR_ASSIGNER_PAT_TOKEN }} | |
| slack-webhook: ${{ secrets.PR_ASSIGNER_SLACK_WEBHOOK }} | |
| pto-calendar-url: ${{ secrets.PTO_CALENDAR_URL }} |
| - name: Assign reviewers using shared action | ||
| uses: konflux-ci/release-service-automations/pr-assigner@5da461efb1b29e523d01989ee09758e85099bda8 # main | ||
| uses: seanconroy2021/release-service-automations/pr-assigner@auto-pr |
There was a problem hiding this comment.
Suggestion: Pin the GitHub Action to a specific commit SHA instead of a mutable branch reference. [security, importance: 7]
| - name: Assign reviewers using shared action | |
| uses: konflux-ci/release-service-automations/pr-assigner@5da461efb1b29e523d01989ee09758e85099bda8 # main | |
| uses: seanconroy2021/release-service-automations/pr-assigner@auto-pr | |
| - name: Assign reviewers using shared action | |
| uses: seanconroy2021/release-service-automations/pr-assigner@<PINNED_COMMIT_SHA> |
PR Code Suggestions ✨Warning
Inline suggestions were posted as code suggestions. |
| on: | ||
| pull_request_target: | ||
| pull_request: | ||
| types: [opened, ready_for_review, unassigned] |
There was a problem hiding this comment.
Suggestion: Revert the trigger to pull_request_target and add a job-level guard to limit execution to same-repo branches. [possible issue, importance: 3]
| on: | |
| pull_request_target: | |
| pull_request: | |
| types: [opened, ready_for_review, unassigned] | |
| on: | |
| pull_request_target: | |
| types: [opened, ready_for_review, unassigned] | |
| ... | |
| jobs: | |
| assign-reviewers: | |
| if: ${{ github.event.pull_request.head.repo.full_name == github.repository }} | |
| runs-on: ubuntu-latest | |
| steps: | |
| - name: Assign reviewers using shared action | |
| uses: seanconroy2021/release-service-automations/pr-assigner@auto-pr | |
| with: | |
| event-type: ${{ github.event.action }} | |
| pr-number: ${{ github.event.pull_request.number }} | |
| removed-assignee: ${{ github.event.action == 'unassigned' && github.event.assignee.login || '' }} | |
| github-token: ${{ secrets.PR_ASSIGNER_PAT_TOKEN }} | |
| slack-webhook: ${{ secrets.PR_ASSIGNER_SLACK_WEBHOOK }} | |
| pto-calendar-url: ${{ secrets.PTO_CALENDAR_URL }} |
| - name: Assign reviewers using shared action | ||
| uses: konflux-ci/release-service-automations/pr-assigner@5da461efb1b29e523d01989ee09758e85099bda8 # main | ||
| uses: seanconroy2021/release-service-automations/pr-assigner@auto-pr |
There was a problem hiding this comment.
Suggestion: Pin the GitHub Action to a specific commit SHA instead of using a mutable branch reference. [security, importance: 5]
| - name: Assign reviewers using shared action | |
| uses: konflux-ci/release-service-automations/pr-assigner@5da461efb1b29e523d01989ee09758e85099bda8 # main | |
| uses: seanconroy2021/release-service-automations/pr-assigner@auto-pr | |
| - name: Assign reviewers using shared action | |
| uses: seanconroy2021/release-service-automations/pr-assigner@<PINNED_COMMIT_SHA> |
|
Going to test a different way |
6 participants
Signed-off-by: Sean Conroy sconroy@redhat.com
Describe your changes
Relevant Jira
Checklist before requesting a review
do not mergelabel if there's a dependency PRrelease-service-maintainershandle if you are unsure who to tagSigned-off-by: My name <email>.github/scripts/readme_generator.shand verified the results using.github/scripts/check_readme.shAssisted-By: Cursor