Feat: Adding fetched AquaSec JSON as workflow artifact.#89
Conversation
WalkthroughThe security scan accepts an optional JSON output path, writes fetched AquaSec findings when configured, and the nightly workflow uploads the file as ChangesAquaSec scan artifact
Estimated code review effort: 2 (Simple) | ~10 minutes Sequence Diagram(s)sequenceDiagram
participant NightlyWorkflow
participant SecurityMain
participant ScanFetcher
participant JSONFile
participant WorkflowArtifact
NightlyWorkflow->>SecurityMain: run with --scan-output aquasec_scan.json
SecurityMain->>ScanFetcher: fetch findings
ScanFetcher-->>SecurityMain: return scan_data
SecurityMain->>JSONFile: write aquasec_scan.json
NightlyWorkflow->>WorkflowArtifact: upload aquasec_scan.json as aquasec-night-scan
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Checkov (3.3.8).github/workflows/aquasec-scan.ymlTraceback (most recent call last): 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: 1
🧹 Nitpick comments (2)
tests/core/test_helpers.py (1)
106-110: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd type hints to the new public test functions.
tests/core/test_helpers.py#L106-L110: annotatetmp_pathasPath.tests/security/test_main.py#L137-L151: annotatemockerasMockerFixtureand add-> Noneto both tests.As per coding guidelines, Python public functions must use type hints.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/core/test_helpers.py` around lines 106 - 110, Add the requested type hints to the new public tests: in tests/core/test_helpers.py lines 106-110, annotate the tmp_path parameter of test_write_json_writes_indented_content as Path; in tests/security/test_main.py lines 137-151, annotate each test’s mocker parameter as MockerFixture and add -> None return annotations to both test functions.Source: Coding guidelines
src/core/helpers.py (1)
54-55: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse a Google-style docstring for the new public helper.
Document
pathanddataunder anArgs:section.Proposed change
def write_json(path: str, data: Any) -> None: - """Write *data* to *path* as indented JSON (UTF-8).""" + """Write data to an indented UTF-8 JSON file. + + Args: + path: Destination file path. + data: JSON-serializable data to write. + """As per coding guidelines, public functions and classes must use Google-style docstrings.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core/helpers.py` around lines 54 - 55, Update the public write_json function’s docstring to Google style by adding an Args: section documenting both path and data, while retaining the existing description of its JSON and UTF-8 output behavior.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/aquasec-scan.yml:
- Around line 136-140: Update the artifact upload step in
.github/workflows/aquasec-scan.yml at lines 136-140 to run on non-cancelled
failures and suppress missing-file warnings. Update the documentation wording in
docs/security/security.md at lines 46-48 to accurately state that every
non-cancelled scan run uploads the artifact.
---
Nitpick comments:
In `@src/core/helpers.py`:
- Around line 54-55: Update the public write_json function’s docstring to Google
style by adding an Args: section documenting both path and data, while retaining
the existing description of its JSON and UTF-8 output behavior.
In `@tests/core/test_helpers.py`:
- Around line 106-110: Add the requested type hints to the new public tests: in
tests/core/test_helpers.py lines 106-110, annotate the tmp_path parameter of
test_write_json_writes_indented_content as Path; in tests/security/test_main.py
lines 137-151, annotate each test’s mocker parameter as MockerFixture and add ->
None return annotations to both test functions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: dcd43623-676c-4f12-8164-f8f3c9f8abc6
📒 Files selected for processing (8)
.github/workflows/aquasec-scan.ymldocs/security/security.mdsrc/core/helpers.pysrc/security/config.pysrc/security/main.pytests/core/test_helpers.pytests/security/test_config.pytests/security/test_main.py
| - name: Upload scan findings artifact | ||
| uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a | ||
| with: | ||
| name: aquasec-night-scan | ||
| path: aquasec_scan.json |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== workflow excerpt =="
if [ -f .github/workflows/aquasec-scan.yml ]; then
nl -ba .github/workflows/aquasec-scan.yml | sed -n '1,220p'
else
echo "missing .github/workflows/aquasec-scan.yml"
fi
echo
echo "== docs excerpt =="
if [ -f docs/security/security.md ]; then
nl -ba docs/security/security.md | sed -n '35,60p'
else
echo "missing docs/security/security.md"
fi
echo
echo "== search relevant actions =="
rg -n "upload-artifact|aquasec_scan|scan Artifact|aquasec-night-scan|if-no-files-found|cancelled\\(\\)" .github docs || true
echo
echo "== git status/diff stat =="
git status --short
git diff --stat HEAD~1..HEAD 2>/dev/null || git diff --stat 2>/dev/null || trueRepository: AbsaOSS/organizational-workflows
Length of output: 233
🏁 Script executed:
#!/bin/sh
set -euo pipefail
echo "== workflow file size and excerpt =="
if [ -f .github/workflows/aquasec-scan.yml ]; then
wc -l .github/workflows/aquasec-scan.yml
awk 'NR>=1 && NR<=220 {printf "%6d\t%s\n", NR, $0}' .github/workflows/aquasec-scan.yml
else
echo "missing .github/workflows/aquasec-scan.yml"
fi
echo
echo "== docs excerpt =="
if [ -f docs/security/security.md ]; then
wc -l docs/security/security.md
awk 'NR>=35 && NR<=60 {printf "%6d\t%s\n", NR, $0}' docs/security/security.md
else
echo "missing docs/security/security.md"
fi
echo
echo "== search relevant actions =="
rg -n "upload-artifact|aquasec_scan|Scan Artifact|aquasec-night-scan|if-no-files-found|cancelled\\(\\)" .github docs || true
echo
echo "== git status/diff stat =="
git status --short || true
git diff --stat HEAD~1..HEAD 2>/dev/null || git diff --stat 2>/dev/null || trueRepository: AbsaOSS/organizational-workflows
Length of output: 10225
🌐 Web query:
actions/upload-artifact v4 behavior when workflow job fails does it upload artifact if files already exist skipped unless continue-on-error
💡 Result:
In GitHub Actions, the actions/upload-artifact@v4 action does not automatically upload files if a job fails; standard steps in a workflow job are skipped when a previous step fails or the job itself enters a failed state [1][2][3]. To ensure an artifact is uploaded even when a job fails, you must explicitly use the if: failure or if: always conditional on the upload step [1][3]. Regarding the behavior of v4: - Artifacts are immutable once created [4][5]. - Uploading to an artifact name that already exists in the same workflow run will cause an error (Conflict: 409) [4][6]. - If you use the overwrite: true option, the action will attempt to delete the existing artifact with that name before creating the new one [7][8]. However, this is not atomic and is intended for serial operations (e.g., modifying a file and re-uploading), not for parallel jobs trying to merge data into a single artifact [9]. - If a job fails, and you have not configured the upload step with an appropriate conditional like if: failure, that step will be skipped and no artifact will be uploaded from that job [1][2]. In summary, the action does not have a built-in mechanism to "catch" and upload artifacts upon failure unless you explicitly configure the step to run under those conditions using GitHub Actions workflow syntax [1][3].
Citations:
- 1: https://github.com/actions/upload-artifact/blob/2a6c27a029e4fff01b17aeb831fdab5f5d97dd6d/README.md
- 2: https://github.com/actions/upload-artifact/blob/9dc0ee81a240d3b68e2f9895bf8b1fa7080a9f8d/README.md
- 3: https://github.com/actions/upload-artifact/blob/9a179cdb8d53d08d8879e064fcc4e6f0ff3414ed/README.md
- 4: https://github.com/actions/upload-artifact?tab=readme-ov-file
- 5: https://github.blog/news-insights/product-news/get-started-with-v4-of-github-actions-artifacts/
- 6: [bug] (v4) Unable to upload to same artifact name from multiple jobs actions/upload-artifact#478
- 7: https://github.com/actions/upload-artifact/blob/v4/README.md
- 8: https://github.com/actions/upload-artifact
- 9: [bug] v4: overwrite: true fails with parallel jobs writing to the artifact actions/upload-artifact#506
🌐 Web query:
actions/upload-artifact if-no-files-found ignore continue run on failed job
💡 Result:
The if-no-files-found input for the actions/upload-artifact GitHub Action is used to control the behavior of the step when no files are matched by the specified path. It accepts three values [1][2]: - warn (default): Prints a warning to the logs but the action completes successfully [1][2]. - error: Fails the action with an error message [1][2]. - ignore: Neither prints a warning nor fails the action [1][2]. This option does not inherently control whether the action runs on a failed job; that is a separate configuration concern [3]. To ensure that an artifact upload step executes even if previous steps in the job have failed, you must use the if: always() or if: failure() conditional expression [3]. Example usage: - uses: actions/upload-artifact@v7 if: always with: name: my-artifact path: path/to/files/ if-no-files-found: ignore In this configuration, the artifact step will always attempt to run (even if earlier steps failed) and will not error or warn if the specified files are missing [1][3][2].
Citations:
- 1: https://github.com/actions/upload-artifact?tab=readme-ov-file
- 2: https://github.com/actions/upload-artifact/blob/main/action.yml
- 3: https://github.com/marketplace/actions/upload-a-build-artifact?version=v2.3.1
Upload fetched findings even when later pipeline stages fail.
main.py writes aquasec_scan.json before parsing, synchronization, and notification, but the artifact step is skipped after a failure by default. As written, the docs line that says every scan run uploads the artifact is only true when the whole job succeeds.
.github/workflows/aquasec-scan.yml#L136-L140: condition the upload on non-cancelled failures and suppress the missing-file warning.docs/security/security.md#L46-L48: either narrow the wording to successful runs or update the docs after making upload failure-safe.
📍 Affects 2 files
.github/workflows/aquasec-scan.yml#L136-L140(this comment)docs/security/security.md#L46-L48
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/aquasec-scan.yml around lines 136 - 140, Update the
artifact upload step in .github/workflows/aquasec-scan.yml at lines 136-140 to
run on non-cancelled failures and suppress missing-file warnings. Update the
documentation wording in docs/security/security.md at lines 46-48 to accurately
state that every non-cancelled scan run uploads the artifact.
1 participant
Overview
This pull request adds the ability to output AquaSec scan findings as a JSON artifact and improves monitoring and traceability of security scans. It introduces a new
--scan-outputoption to the security pipeline, writes scan results to a file when requested, uploads the artifact in CI, and documents the feature. Comprehensive tests are included to ensure correct behavior.Release Notes
Related
Closes #88
Summary by CodeRabbit
New Features
--scan-outputoption to save scan findings as formatted JSON.aquasec-night-scanworkflow artifact.Documentation
Tests