CICD 46 Add Code Formatter And CI/CD #124
Conversation
|
Please let me know any desired changes that are needed. Thank you! |
|
Hello, I wanted to follow up on the pull request that I made since it's been a couple of weeks since I submitted it. Please let me know if there’s anything more I can do to assist or if there are any changes you’d like me to make. I truly appreciate your time and effort in reviewing this when possible. Thank you! |
There was a problem hiding this comment.
Pull Request Overview
This PR integrates Laravel Pint as a code formatter and establishes CI/CD linting practices by configuring formatting tools and updating the linter workflow. The changes primarily focus on code style consistency by converting class constants to use explicit visibility modifiers and implementing automated formatting checks.
- Adds Laravel Pint configuration with PSR-12 preset for consistent code formatting
- Updates all class constants to use explicit
publicvisibility modifiers across storage-related classes - Modifies CI/CD workflow to use a simpler checkout process for linting operations
Reviewed Changes
Copilot reviewed 11 out of 12 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/Storage/Validator/FileType.php | Converts class constants to use explicit public visibility |
| src/Storage/Validator/FileExt.php | Converts class constants to use explicit public visibility |
| src/Storage/Storage.php | Converts device constants to use explicit public visibility |
| src/Storage/Device/Wasabi.php | Converts region constants to use explicit public visibility |
| src/Storage/Device/S3.php | Converts HTTP method, region, and ACL constants to public visibility; fixes stdClass instantiation |
| src/Storage/Device/Linode.php | Converts region constants to use explicit public visibility |
| src/Storage/Device/DOSpaces.php | Converts region constants to use explicit public visibility |
| src/Storage/Device/Backblaze.php | Converts region constants to use explicit public visibility |
| pint.json | Adds Laravel Pint configuration with PSR-12 preset |
| composer.json | Updates linting scripts to use pint.json config and adds phpstan dependency |
| .github/workflows/linter.yml | Simplifies checkout process for CI/CD linting |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
.github/workflows/linter.yml
Outdated
| with: | ||
| fetch-depth: 2 | ||
| - name: Check out the repo | ||
| uses: actions/checkout@v2 |
There was a problem hiding this comment.
Using actions/checkout@v2 is outdated and has known security vulnerabilities. Update to actions/checkout@v4 or the latest stable version for better security and performance.
| uses: actions/checkout@v2 | |
| uses: actions/checkout@v4 |
| public const EU_NORTH_1 = 'eu-north-1'; | ||
|
|
||
| const SA_EAST_1 = 'eu-north-1'; | ||
| public const SA_EAST_1 = 'eu-north-1'; |
There was a problem hiding this comment.
The SA_EAST_1 constant has an incorrect value. It should be 'sa-east-1' but is set to 'eu-north-1', which is the value for EU_NORTH_1.
| public const SA_EAST_1 = 'eu-north-1'; | |
| public const SA_EAST_1 = 'sa-east-1'; |
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughMade many class constants explicitly public across storage devices and validators, added a root Changes
Sequence Diagram(s)(Skipped — changes are declaration/CI/tooling edits and a trivial instantiation tweak; no control-flow or feature-level interaction to diagram.) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
✨ Finishing Touches🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
.github/workflows/linter.yml (1)
11-11: Upgrade to actions/checkout@v4 (v2 is EOL and blocked on modern runners).
Actionlint flags v2 as too old; also previously noted in review.Apply this diff:
- uses: actions/checkout@v2 + uses: actions/checkout@v4src/Storage/Device/S3.php (1)
40-91: Incorrect AWS region value for SA_EAST_1SA_EAST_1 currently maps to 'eu-north-1'. It should be 'sa-east-1'.
Apply this fix:
- public const SA_EAST_1 = 'eu-north-1'; + public const SA_EAST_1 = 'sa-east-1';
🧹 Nitpick comments (10)
pint.json (1)
1-6: PSR-12 config looks good; optionally exclude vendor to speed runs.
Small optimization to avoid scanning dependencies during local runs/IDE hooks.Apply this diff:
{ "preset": "psr12", "exclude": [ - "tests/resources" + "tests/resources", + "vendor" ] }composer.json (1)
33-35: PHPStan is added but unused; either wire it into scripts/CI or drop it.
Keeps dev deps purposeful and CI consistent.Apply this diff to add a PHPStan script:
"scripts": { - "lint": "./vendor/bin/pint --test --config pint.json", - "format": "./vendor/bin/pint --config pint.json" + "lint": "./vendor/bin/pint --test --config pint.json", + "format": "./vendor/bin/pint --config pint.json", + "stan": "php -d memory_limit=-1 ./vendor/bin/phpstan analyse --no-interaction --ansi" },If you prefer to keep this PR focused strictly on formatting, consider removing phpstan/phpstan for now.
.github/workflows/linter.yml (2)
13-16: Pin the Composer image tag for reproducibility.
Avoids unexpected breaks when thelatesttag moves.Apply this diff:
- run: | - docker run --rm -v $PWD:/app composer sh -c \ + run: | + docker run --rm -v "$PWD":/app composer:2 sh -c \ "composer install --profile --ignore-platform-reqs && composer lint"
16-16: Add a newline at end of file.
Satisfies YAMLlint and POSIX tooling expectations.src/Storage/Device/DOSpaces.php (1)
12-22: Lowercase region slug for AMS3Region values are lowercase elsewhere (sgp1, nyc3, fra1, sfo2, sfo3). Recommend lowercasing AMS3 for consistency and to avoid surprises in string comparisons.
Apply:
- public const AMS3 = 'AMS3'; + public const AMS3 = 'ams3';src/Storage/Validator/FileExt.php (1)
9-17: Make TYPE_ZIP visibility explicit for consistencyAlign TYPE_ZIP with the others.
Apply:
- const TYPE_ZIP = 'zip'; + public const TYPE_ZIP = 'zip';src/Storage/Device/S3.php (4)
919-919: FQN for curl_setopt for consistency/perfElsewhere you use fully qualified \curl_*; align this call too.
Apply:
- curl_setopt($curl, CURLOPT_HEADERFUNCTION, function ($curl, string $header) use (&$response) { + \curl_setopt($curl, CURLOPT_HEADERFUNCTION, function ($curl, string $header) use (&$response) {
872-872: Avoid trailing ? in URL when no query paramsPrevents requests like https://host/path?.
- $url = 'https://'.$this->headers['host'].$uri.'?'.\http_build_query($parameters, '', '&', PHP_QUERY_RFC3986); + $query = \http_build_query($parameters, '', '&', PHP_QUERY_RFC3986); + $url = 'https://'.$this->headers['host'].$uri.($query !== '' ? '?'.$query : '');
509-509: Nit: fix TODO wordingTiny spelling/grammar tweak.
- $this->headers['content-md5'] = \base64_encode(md5($data, true)); //TODO whould this work well with big file? can we skip it? + $this->headers['content-md5'] = \base64_encode(md5($data, true)); // TODO would this work well with big files? Can we skip it?
806-809: Nit: remove unnecessary reference in callableUsing [&$this, ...] is unusual; [$this, ...] is sufficient.
- uksort($combinedHeaders, [&$this, 'sortMetaHeadersCmp']); + uksort($combinedHeaders, [$this, 'sortMetaHeadersCmp']); - uksort($parameters, [&$this, 'sortMetaHeadersCmp']); + uksort($parameters, [$this, 'sortMetaHeadersCmp']);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (11)
.github/workflows/linter.yml(1 hunks)composer.json(2 hunks)pint.json(1 hunks)src/Storage/Device/Backblaze.php(1 hunks)src/Storage/Device/DOSpaces.php(1 hunks)src/Storage/Device/Linode.php(1 hunks)src/Storage/Device/S3.php(2 hunks)src/Storage/Device/Wasabi.php(1 hunks)src/Storage/Storage.php(1 hunks)src/Storage/Validator/FileExt.php(1 hunks)src/Storage/Validator/FileType.php(1 hunks)
🧰 Additional context used
🪛 actionlint (1.7.7)
.github/workflows/linter.yml
11-11: the runner of "actions/checkout@v2" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
🪛 YAMLlint (1.37.1)
.github/workflows/linter.yml
[error] 16-16: no new line character at the end of file
(new-line-at-end-of-file)
🔇 Additional comments (12)
composer.json (1)
15-16: Scripts LGTM.
Clear split between check (lint) and write (format).src/Storage/Validator/FileType.php (1)
13-19: Making constants explicitly public is fine and consistent.
No behavior change (constants were effectively public already); improves clarity across validators.src/Storage/Device/Linode.php (1)
12-18: Public visibility on region constants LGTM.
Aligns with other devices; no functional impact.src/Storage/Storage.php (1)
12-22: Explicit public constants — LGTMMaking device constants explicitly public improves clarity without changing behavior.
src/Storage/Device/DOSpaces.php (1)
12-22: Public visibility standardization — LGTMExposing region constants explicitly as public is consistent with the rest of the adapters.
src/Storage/Validator/FileExt.php (1)
9-17: Explicit public constants — LGTMConsistent with the project-wide visibility updates.
src/Storage/Device/Backblaze.php (1)
15-23: Public region constants — LGTMValues and defaults look correct for Backblaze clusters.
src/Storage/Device/Wasabi.php (1)
12-30: Public region constants — LGTMGood consistency across regions; no behavioral change.
src/Storage/Device/S3.php (4)
11-28: Explicit constant visibility for HTTP methods — LGTMNo behavior change; improves clarity and static analysis.
29-35: Expose cURL HTTP version constants — LGTMKeeps version selection explicit via setHttpVersion().
95-101: ACL constants visibility — LGTMConsistent with other constants; no functional change.
873-873: Minor: stdClass instantiation — LGTMUsing parentheses is fine and consistent with the rest of the file.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
.github/workflows/linter.yml (1)
11-11: Checkout v4 upgrade resolves prior concern.Moving to actions/checkout@v4 addresses the outdated v2 note from earlier reviews. Nice.
🧹 Nitpick comments (2)
.github/workflows/linter.yml (2)
11-13: Pin actions to a full commit SHA for supply‑chain safety.Recommend pinning checkout to a specific v4 SHA (and optionally adding a comment with the tag) to avoid tag‑hijack.
Apply:
- uses: actions/checkout@v4 + uses: actions/checkout@v4 # pin@<commit-sha>Optionally:
with: fetch-depth: 2 + # If you need the PR head explicitly: + # ref: ${{ github.event.pull_request.head.sha }}
15-15: Avoid brittlegit checkout HEAD^2.This relies on a merge commit layout and can break. Prefer letting checkout grab the desired ref and drop this step.
Apply:
- - run: git checkout HEAD^2If you need the PR head explicitly, set it on checkout:
with: fetch-depth: 2 + ref: ${{ github.event.pull_request.head.sha }}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
.github/workflows/linter.yml(1 hunks)
🧰 Additional context used
🪛 YAMLlint (1.37.1)
.github/workflows/linter.yml
[error] 17-17: syntax error: expected , but found '-'
(syntax)
|
Hello, I merged the Copilot fixes into this branch after looking at what Copilot suggested: Please take a look and let me know if anything else should be fixed. Thank you! |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
.github/workflows/linter.yml (1)
11-11: Good fix: upgraded to actions/checkout@v4.This addresses the prior security and deprecation notice.
🧹 Nitpick comments (4)
.github/workflows/linter.yml (4)
15-15: Prefer checking out the PR head via checkout.ref instead ofgit checkout HEAD^2.Using
ref: ${{ github.event.pull_request.head.sha }}is clearer and avoids reliance on merge commit ancestry depth. Remove the manualgit checkoutstep.Apply:
- name: Checkout repository - uses: actions/checkout@v4 - with: - fetch-depth: 2 - - - run: git checkout HEAD^2 + uses: actions/checkout@v4 + with: + fetch-depth: 2 + ref: ${{ github.event.pull_request.head.sha }}
18-20: Trim trailing spaces and add a final newline to satisfy YAML linters.YAMLlint flags trailing spaces on Line 18 and missing newline at EOF.
Apply:
- run: | + run: | docker run --rm -v $PWD:/app composer sh -c \ "composer install --profile --ignore-platform-reqs && composer lint" +
19-20: Pin the Composer image/tag (or digest) for reproducibility and supply-chain safety.Unpinned
composeris mutable. Pin tocomposer:2(or a specific patch or digest).Apply:
- docker run --rm -v $PWD:/app composer sh -c \ - "composer install --profile --ignore-platform-reqs && composer lint" + docker run --rm -v "$PWD":/app composer:2 sh -lc \ + 'composer install --profile --ignore-platform-reqs && composer lint'
1-7: Set explicit, minimal GITHUB_TOKEN permissions.Be explicit to reduce attack surface in PRs from forks.
Apply:
name: "Linter" on: [pull_request] +permissions: + contents: read jobs: lint: name: Linter
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
.github/workflows/linter.yml(1 hunks)
🧰 Additional context used
🪛 YAMLlint (1.37.1)
.github/workflows/linter.yml
[error] 18-18: trailing spaces
(trailing-spaces)
[error] 20-20: no new line character at the end of file
(new-line-at-end-of-file)
1 participant
Made the following changes via these three steps:
Summary by CodeRabbit