Skip to content
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.

Sign up for GitHub

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

Jump to bottom

refactor(github_scripts): improve code formatting #74

Merged
merged 2 commits into from
Jan 29, 2025
Merged

refactor(github_scripts): improve code formatting #74

merged 2 commits into from
Jan 29, 2025

Conversation

jacopocarlini
Copy link
Contributor

List of Changes

✨ Removed unused code and improved code formatting for better readability.

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as
    expected)

Checklist:

  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.

@jacopocarlini
refactor(github_scripts): improve code formatting and remove unnecess…
dee2980 
…ary code\n \n ✨ Removed unused code and improved code formatting for better readability.\n
@jacopocarlini jacopocarlini requested a review from a team as a code owner January 24, 2025 09:10
@github-actions GitHub Actions
Copy link

The default action is to increase the PATCH number of SEMVER. Set IGNORE-FOR-RELEASE if you want to skip SEMVER bump. BREAKING-CHANGE and NEW-RELEASE must be run from GH Actions section manually.

@github-actions GitHub Actions
Copy link

This pull request does not contain a valid label. Please add one of the following labels: [major, minor, patch, patch, skip]

@github-actions github-actions bot added the patch label Jan 24, 2025
@github-actions GitHub Actions
Copy link

The default action is to increase the PATCH number of SEMVER. Set IGNORE-FOR-RELEASE if you want to skip SEMVER bump. BREAKING-CHANGE and NEW-RELEASE must be run from GH Actions section manually.

github-advanced-security[bot]
github-advanced-security bot found potential problems Jan 24, 2025
View reviewed changes
.github/workflows/github_scripts/check_size.js
const execSync = require('child_process').execSync;
for (const file of IGNORED_FILES.trim().split(',')) {

const ignored_additions_str = execSync('git --no-pager diff --numstat origin/main..origin/' + BRANCH_NAME + ' | grep ' + file + ' | cut -f 1', {encoding: 'utf-8'})

Check warning

Code scanning / CodeQL

Indirect uncontrolled command line Medium

This command depends on an unsanitized
environment variable
Loading
.

Copilot Autofix AI 12 days ago

To fix the problem, we should avoid using execSync with a concatenated string and instead use execFileSync which accepts command arguments as an array of strings. This approach is safer and prevents command injection vulnerabilities. We will need to modify the lines where execSync is used to execute the git commands and replace them with execFileSync.

Suggested changeset 1
.github/workflows/github_scripts/check_size.js

Autofix patch

Autofix patch
Run the following command in your local git repository to apply this patch
cat << 'EOF' | git apply
diff --git a/.github/workflows/github_scripts/check_size.js b/.github/workflows/github_scripts/check_size.js
--- a/.github/workflows/github_scripts/check_size.js
+++ b/.github/workflows/github_scripts/check_size.js
@@ -10,7 +10,15 @@
         var ignored = 0
-        const execSync = require('child_process').execSync;
+        const { execFileSync } = require('child_process');
         for (const file of IGNORED_FILES.trim().split(',')) {
 
-            const ignored_additions_str = execSync('git --no-pager  diff --numstat origin/main..origin/' + BRANCH_NAME + ' | grep ' + file + ' | cut -f 1', {encoding: 'utf-8'})
-            const ignored_deletions_str = execSync('git --no-pager  diff --numstat origin/main..origin/' + BRANCH_NAME + ' | grep ' + file + ' | cut -f 2', {encoding: 'utf-8'})
+            const ignored_additions_str = execFileSync('git', ['--no-pager', 'diff', '--numstat', `origin/main..origin/${BRANCH_NAME}`], { encoding: 'utf-8' })
+                .split('\n')
+                .filter(line => line.includes(file))
+                .map(line => line.split('\t')[0])
+                .join('\n');
+            const ignored_deletions_str = execFileSync('git', ['--no-pager', 'diff', '--numstat', `origin/main..origin/${BRANCH_NAME}`], { encoding: 'utf-8' })
+                .split('\n')
+                .filter(line => line.includes(file))
+                .map(line => line.split('\t')[1])
+                .join('\n');
 
EOF
@@ -10,7 +10,15 @@
var ignored = 0
const execSync = require('child_process').execSync;
const { execFileSync } = require('child_process');
for (const file of IGNORED_FILES.trim().split(',')) {

const ignored_additions_str = execSync('git --no-pager diff --numstat origin/main..origin/' + BRANCH_NAME + ' | grep ' + file + ' | cut -f 1', {encoding: 'utf-8'})
const ignored_deletions_str = execSync('git --no-pager diff --numstat origin/main..origin/' + BRANCH_NAME + ' | grep ' + file + ' | cut -f 2', {encoding: 'utf-8'})
const ignored_additions_str = execFileSync('git', ['--no-pager', 'diff', '--numstat', `origin/main..origin/${BRANCH_NAME}`], { encoding: 'utf-8' })
.split('\n')
.filter(line => line.includes(file))
.map(line => line.split('\t')[0])
.join('\n');
const ignored_deletions_str = execFileSync('git', ['--no-pager', 'diff', '--numstat', `origin/main..origin/${BRANCH_NAME}`], { encoding: 'utf-8' })
.split('\n')
.filter(line => line.includes(file))
.map(line => line.split('\t')[1])
.join('\n');

Copilot is powered by AI and may make mistakes. Always verify output.
Positive Feedback
Negative Feedback

Provide additional feedback

Please help us improve GitHub Copilot by sharing more details about this comment.

Please select one or more of the options
.github/workflows/github_scripts/check_size.js
for (const file of IGNORED_FILES.trim().split(',')) {

const ignored_additions_str = execSync('git --no-pager diff --numstat origin/main..origin/' + BRANCH_NAME + ' | grep ' + file + ' | cut -f 1', {encoding: 'utf-8'})
const ignored_deletions_str = execSync('git --no-pager diff --numstat origin/main..origin/' + BRANCH_NAME + ' | grep ' + file + ' | cut -f 2', {encoding: 'utf-8'})

Check warning

Code scanning / CodeQL

Indirect uncontrolled command line Medium

This command depends on an unsanitized
environment variable
Loading
.
@sonarqubecloud SonarQubeCloud
Copy link

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

andrea-deri
andrea-deri approved these changes Jan 29, 2025
View reviewed changes
Copy link
Contributor

@andrea-deri andrea-deri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚀

@github-actions GitHub Actions
Copy link

This PR exceeds the recommended size of 400 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR might be rejected due to its size.

@andrea-deri andrea-deri merged commit 0628f38 into main Jan 29, 2025
8 of 10 checks passed
@andrea-deri andrea-deri deleted the code-review-gha branch January 29, 2025 07:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@andrea-deri andrea-deri andrea-deri approved these changes

Assignees

@jacopocarlini jacopocarlini

Labels
ignore-for-release size/large skip
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@jacopocarlini @andrea-deri