-
Notifications
You must be signed in to change notification settings - Fork 0
ci: Add analyze #49
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
ci: Add analyze #49
Changes from all commits
f648dfa
4455ba4
91c1eef
a27368a
3a21007
b72b0f6
cc99be7
8548df6
f9507e8
24383aa
78dcfe6
d893057
2a3213e
9052c3b
7421bf4
26e1d54
5604086
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,54 @@ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| name: "PR Analysis" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| on: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| workflow_run: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| workflows: ["Sync fabricv4 API spec"] | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| types: [completed] | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| permissions: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| contents: read | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| pull-requests: write | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| actions: read | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| jobs: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| pr-analysis: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| name: Branch File Analysis | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| runs-on: ubuntu-latest | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if: github.event.workflow_run.conclusion == 'success' | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| steps: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| - name: Checkout code | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| uses: actions/checkout@v4 | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+21
to
+22
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| uses: actions/checkout@v4 | |
| uses: actions/checkout@v4 | |
| with: | |
| ref: ${{ github.event.workflow_run.head_branch }} | |
| - name: Fetch base branch | |
| run: git fetch origin main:main |
Copilot
AI
Feb 13, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The GitHub CLI is being manually installed instead of using the official GitHub-provided action. GitHub-hosted runners already have gh CLI pre-installed, so lines 30-34 (installing gh) are unnecessary and add execution time.
You can remove these lines and directly use the gh command. If you need a specific version, you can use the gh version that comes with the runner or explicitly verify the version is available.
Copilot
AI
Feb 13, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The workflow uses secrets.GITHUB_TOKEN for authentication at line 35. However, when using workflow_run triggers, the GITHUB_TOKEN in the triggered workflow has limited permissions and may not have write access to pull requests created from forks or in certain repository configurations.
Consider using a GitHub App token or a Personal Access Token (PAT) with appropriate permissions, stored as a separate secret, to ensure the workflow can update PRs reliably.
Copilot
AI
Feb 13, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The workflow lacks error handling for the case when no PR is found for the branch. If PR_NUMBER is empty (which can happen if the branch has no open PR), subsequent commands using $PR_NUMBER will fail with unclear errors.
Add a check after line 39:
if [ -z "$PR_NUMBER" ]; then
echo "No open PR found for branch $BRANCH"
exit 0
fi| PR_NUMBER=$(gh pr list --head "$BRANCH" --state open --json number --jq '.[0].number // empty') | |
| PR_NUMBER=$(gh pr list --head "$BRANCH" --state open --json number --jq '.[0].number // empty') | |
| if [ -z "$PR_NUMBER" ]; then | |
| echo "No open PR found for branch $BRANCH" | |
| exit 0 | |
| fi |
Copilot
AI
Feb 13, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The workflow doesn't handle the case where the Python script produces no output or fails silently. If ANALYSIS_RESULT is empty, the PR description will be updated with an empty analysis section, which could be confusing.
Add validation after line 42:
if [ -z "$ANALYSIS_RESULT" ]; then
echo "No analysis results generated"
exit 0
fi| ANALYSIS_RESULT=$(python3 script/branch_file_analyzer.py) | |
| ANALYSIS_RESULT=$(python3 script/branch_file_analyzer.py) | |
| if [ -z "$ANALYSIS_RESULT" ]; then | |
| echo "No analysis results generated" | |
| exit 0 | |
| fi |
Copilot
AI
Feb 13, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The sed command to remove the old analysis section may not work correctly in all cases. The regex pattern /^\*Auto-generated by Branch File Analysis workflow\*$/d looks for an exact line match, but the actual line in the PR description (from line 51) is *Auto-generated by Branch File Analysis workflow* without the ^ and $ being part of the literal text.
The sed command should use a simpler pattern or ensure proper escaping. Additionally, the range deletion may fail if the closing marker is not found, leaving partial content. Consider using a more robust approach with a unique marker or using a script that handles edge cases better.
| NEW_DESC=$(echo "$CURRENT_DESC" | sed '/## 📋 Branch File Analysis/,/^\*Auto-generated by Branch File Analysis workflow\*$/d') | |
| NEW_DESC=$(printf "%s" "$CURRENT_DESC" | python3 - << 'EOF' | |
| import sys | |
| body = sys.stdin.read() | |
| start = "## 📋 Branch File Analysis" | |
| end = "*Auto-generated by Branch File Analysis workflow*" | |
| lines = body.splitlines() | |
| out = [] | |
| buffer = [] | |
| in_block = False | |
| for line in lines: | |
| if not in_block: | |
| if line.strip() == start: | |
| in_block = True | |
| buffer = [line] | |
| else: | |
| out.append(line) | |
| else: | |
| buffer.append(line) | |
| if line.strip() == end: | |
| # Completed a block; drop it and reset state | |
| in_block = False | |
| buffer = [] | |
| # If we were in a block but never found the end marker, restore the buffered lines | |
| if in_block and buffer: | |
| out.extend(buffer) | |
| print("\n".join(out)) | |
| EOF | |
| ) |
Copilot
AI
Feb 13, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The sed command at line 48 and the printf command at line 51 process user-generated content (PR descriptions) without proper escaping. If a PR description contains special characters like backticks, dollar signs, or backslashes, it could lead to command injection or unexpected behavior.
Consider using a more robust method to manipulate the PR description, such as:
- Writing the content to temporary files instead of using shell variables
- Using a Python or other scripting language that handles strings more safely
- Properly escaping the content before using it in shell commands
This is especially important since the PR description could potentially be modified by attackers if they have write access to the repository.
| # Remove old analysis section if exists | |
| NEW_DESC=$(echo "$CURRENT_DESC" | sed '/## 📋 Branch File Analysis/,/^\*Auto-generated by Branch File Analysis workflow\*$/d') | |
| # Add new analysis section | |
| UPDATED_DESC=$(printf "%s\n\n## 📋 Branch File Analysis\n\n%s\n\n---\n*Auto-generated by Branch File Analysis workflow*" "$NEW_DESC" "$ANALYSIS_RESULT") | |
| # Update PR | |
| gh pr edit $PR_NUMBER --body "$UPDATED_DESC" | |
| # Export variables for Python script | |
| export ANALYSIS_RESULT | |
| export CURRENT_DESC | |
| # Use Python to safely update the PR description and write it to a file | |
| python3 - << 'PY' | |
| import os | |
| import re | |
| current_desc = os.environ.get("CURRENT_DESC", "") | |
| analysis_result = os.environ.get("ANALYSIS_RESULT", "") | |
| # Remove existing "Branch File Analysis" section, if present | |
| pattern = ( | |
| r"\n?## 📋 Branch File Analysis" | |
| r".*?" | |
| r"\*Auto-generated by Branch File Analysis workflow\*" | |
| r"\n?" | |
| ) | |
| new_desc = re.sub(pattern, "", current_desc, flags=re.DOTALL) | |
| new_desc = new_desc.rstrip() | |
| updated_desc = ( | |
| (new_desc + "\n\n" if new_desc else "") | |
| + "## 📋 Branch File Analysis\n\n" | |
| + analysis_result | |
| + "\n\n---\n*Auto-generated by Branch File Analysis workflow*" | |
| ) | |
| with open("updated_desc.txt", "w", encoding="utf-8") as f: | |
| f.write(updated_desc) | |
| PY | |
| # Update PR using the file to avoid shell interpolation issues | |
| gh pr edit $PR_NUMBER --body-file updated_desc.txt |
Copilot
AI
Feb 13, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The shell script uses unquoted variables in several places (e.g., $PR_NUMBER, $ANALYSIS_RESULT, $NEW_DESC). If any of these variables contain special characters or whitespace, the commands could fail or behave unexpectedly.
While GitHub Actions usually sets set -e by default (which helps catch errors), it's still a best practice to quote variables, especially when they contain user-generated content like PR descriptions. Consider quoting variables throughout:
PR_NUMBER=$(gh pr list --head "$BRANCH" --state open --json number --jq '.[0].number // empty')
CURRENT_DESC=$(gh pr view "$PR_NUMBER" --json body --jq '.body // ""')
Copilot
AI
Feb 13, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The entire workflow runs in a single step with a multi-line bash script. If any command in the middle fails (like the gh CLI installation or the Python script), the workflow will fail without clear indication of which specific step failed. This makes debugging difficult.
Consider splitting this into multiple named steps (e.g., "Install GitHub CLI", "Get PR Number", "Run Analysis", "Update PR Description") so that GitHub Actions can provide better visibility into which step failed and make the workflow easier to maintain.
| Original file line number | Diff line number | Diff line change | ||||||
|---|---|---|---|---|---|---|---|---|
|
|
@@ -37,7 +37,6 @@ public void getMetroByCode() throws ApiException { | |||||||
| public void getMetros() throws ApiException { | ||||||||
| MetroResponse metroResponse = metrosApi.getMetros(null,1, 10); | ||||||||
| assertEquals(200, metrosApi.getApiClient().getStatusCode()); | ||||||||
| boolean metroFound = metroResponse.getData().stream().anyMatch(metro -> metro.getCode().equals(metroCode)); | ||||||||
| assertTrue(metroFound); | ||||||||
| assertTrue(!metroResponse.getData().isEmpty()); | ||||||||
|
||||||||
| assertTrue(!metroResponse.getData().isEmpty()); | |
| boolean metroFound = metroResponse.getData().stream().anyMatch(metro -> metro.getCode().equals(metroCode)); | |
| assertTrue(metroFound); |
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,297 @@ | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| #!/usr/bin/env python3 | ||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||
| import subprocess | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| import sys | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| import os | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| from pathlib import Path | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| from collections import defaultdict | ||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||
| class BranchFileAnalyzer: | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| def __init__(self, base_branch='main'): | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| self.base_branch = base_branch | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| self.current_branch = self._get_current_branch() | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| self.base_ref = self._get_base_ref() | ||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||
| # File categorization | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| self.added_files = [] | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| self.modified_files = [] | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| self.deleted_files = [] | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| self.renamed_files = {} | ||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||
| def _run_git_command(self, cmd): | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| """Execute git command and return output.""" | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| try: | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| result = subprocess.run(cmd, capture_output=True, text=True, check=True, cwd='.') | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| return result.stdout.strip() | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| except subprocess.CalledProcessError: | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| return "" | ||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+27
to
+28
|
||||||||||||||||||||||||||||||||||||||||||||||||||||
| except subprocess.CalledProcessError: | |
| return "" | |
| except subprocess.CalledProcessError as e: | |
| print( | |
| f"Warning: git command failed: {cmd} (return code {e.returncode}). " | |
| f"stderr: {e.stderr.strip() if e.stderr else 'None'}", | |
| file=sys.stderr, | |
| ) | |
| return "" | |
| except FileNotFoundError as e: | |
| # git executable not found | |
| print( | |
| f"Error: git command not found while running: {cmd}. " | |
| f"Details: {e}", | |
| file=sys.stderr, | |
| ) | |
| return "" |
Copilot
AI
Feb 13, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The categorize_files_by_type method (lines 127-151) is defined but never used anywhere in the code. This dead code should be removed to improve code maintainability and avoid confusion.
If this method is intended for future use, consider removing it for now and adding it back when it's actually needed.
| def categorize_files_by_type(self, file_list): | |
| """Categorize files by their type/extension.""" | |
| categories = defaultdict(list) | |
| for filepath in file_list: | |
| file_path = Path(filepath) | |
| filename = file_path.name | |
| # Categorize by file type | |
| if filepath.endswith('.java'): | |
| if '/model/' in filepath or '/dto/' in filepath: | |
| categories['Java Models'].append(filename) | |
| elif '/api/' in filepath and filename.endswith('Api.java'): | |
| categories['API Classes'].append(filename) | |
| elif '/test/' in filepath or 'test' in filename.lower(): | |
| categories['Test Files'].append(filename) | |
| # Removed Java Files category - these files will be ignored | |
| elif filepath.endswith(('.py', '.sh', '.js', '.ts')): | |
| categories['Scripts'].append(filename) | |
| elif filepath.endswith(('.json', '.xml')): | |
| categories['Data Files'].append(filename) | |
| else: | |
| categories['Other Files'].append(filename) | |
| return categories |
Copilot
AI
Feb 13, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The condition ('/api/' in f and f.endswith('Api.java')) is redundant because it checks f.endswith('Api.java') when you've already checked f.endswith('.java') in the outer condition. This pattern repeats on lines 161, 185, 209, 238, and 239.
The inner check f.endswith('Api.java') is sufficient and more specific than the combined check. Consider simplifying to:
added_apis = [f for f in self.added_files if '/api/' in f and f.endswith('Api.java')]This removes the redundant .endswith('.java') check and makes the code clearer.
| added_apis = [f for f in self.added_files if f.endswith('.java') and ('/api/' in f and f.endswith('Api.java'))] | |
| added_apis = [f for f in self.added_files if '/api/' in f and f.endswith('Api.java')] |
Copilot
AI
Feb 13, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code has significant duplication in the report generation section. The logic for filtering and formatting added, modified, deleted, and renamed files follows the same pattern but is repeated four times (lines 159-180, 183-204, 207-228, 231-267).
Consider extracting a helper method to reduce duplication:
def _format_file_section(self, title, files, symbol):
apis = [f for f in files if '/api/' in f and f.endswith('Api.java')]
models = [f for f in files if '/model/' in f or '/dto/' in f and f.endswith('.java')]
# ... format and return linesThis would improve maintainability and reduce the risk of inconsistent changes across sections.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The workflow is triggered by the completion of "Sync fabricv4 API spec" workflow but doesn't verify that the workflow actually created or updated a PR. The workflow will run even if the sync workflow completed successfully but didn't create a PR (for example, if there were no changes to sync).
Consider adding a check to verify that the sync workflow actually created or found a PR before proceeding with the analysis. This could be done by checking if
PR_NUMBERis empty after line 39, but it would be better to check earlier in the workflow to avoid unnecessary setup steps.