Pending changes exported from your codespace - #293
Conversation
There was a problem hiding this comment.
Pull request overview
This PR modernizes and improves GitHub Actions workflow configurations across the repository. The changes focus on updating action versions to their latest releases, adding conditional execution to prevent failures when dependencies aren't configured, and improving code formatting consistency in embedded Python scripts.
Key changes include:
- Updating GitHub Actions to use version tags (v3, v4, etc.) instead of commit SHAs for better maintainability
- Adding conditional execution guards using repository variables to skip workflows when external services aren't configured
- Migrating the ESLint workflow from npm to pnpm for package management
- Refactoring the ci-cd.yml workflow to remove matrix strategies and consolidate component-specific logic
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 15 comments.
Show a summary per file
| File | Description |
|---|---|
.github/workflows/taskspec-validate.yml |
Reformats inline Python script with better indentation and extracts base ref to environment variable |
.github/workflows/sonarcloud.yml |
Adds conditional execution check, updates action to v3, provides configurable project keys with defaults |
.github/workflows/rust-clippy.yml |
Migrates from deprecated actions-rs/toolchain to dtolnay/rust-toolchain@stable |
.github/workflows/pyre.yml |
Adds path filters for Python files, creates requirements.txt if missing, updates action version |
.github/workflows/policy-validator-tf.yml |
Adds path filters for Terraform files, converts env vars to use repository variables, updates actions to version tags |
.github/workflows/pmd.yml |
Adds Java file detection, updates JDK from 11 to 17, dynamically determines source path |
.github/workflows/ossar.yml |
Uncomments and updates .NET setup from 3.1.x to 6.0.x |
.github/workflows/hadolint.yml |
Adds Dockerfile discovery logic and conditional execution based on file existence |
.github/workflows/frogbot-scan-and-fix.yml |
Adds conditional execution guard and updates to v2 version tag |
.github/workflows/ethicalcheck.yml |
Removes scheduled runs, adds OpenAPI spec discovery, makes configuration more flexible |
.github/workflows/eslint.yml |
Migrates from npm to pnpm, adds proper Node.js setup with caching |
.github/workflows/duplicate-fence.yml |
Reformats inline Python script and extracts base ref to environment variable |
.github/workflows/ci.yml |
Updates guardrail step names from colon to dash separator for consistency |
.github/workflows/ci-cd.yml |
Major refactor removing matrix strategies, updating action versions, adding error suppression to optional checks |
.github/workflows/anchore-syft.yml |
Adds Dockerfile existence check for specific service path and updates action version |
| - name: Install .NET | ||
| uses: actions/setup-dotnet@v4 | ||
| with: | ||
| dotnet-version: '6.0.x' |
There was a problem hiding this comment.
The .NET version has been updated from 3.1.x to 6.0.x while the comment still refers to version 3.1.201. The comment should be updated to reflect the new .NET 6.0 requirement or removed if no longer relevant. Additionally, verify that OSSAR is compatible with .NET 6.0.
| java_files=$(find . -name "*.java" -type f | head -1) | ||
| if [ -n "$java_files" ]; then | ||
| echo "found=true" >> $GITHUB_OUTPUT | ||
| echo "source_path=$(dirname $java_files)" >> $GITHUB_OUTPUT | ||
| else |
There was a problem hiding this comment.
The logic to find Java source files is incorrect. The script finds only one Java file and extracts its directory, but PMD needs to analyze a directory containing all Java sources, not just a single file's directory. If Java files are in multiple directories (e.g., src/main/java, src/test/java), only the first one found will be scanned. Consider using a common base directory or finding the appropriate source root.
| java_files=$(find . -name "*.java" -type f | head -1) | |
| if [ -n "$java_files" ]; then | |
| echo "found=true" >> $GITHUB_OUTPUT | |
| echo "source_path=$(dirname $java_files)" >> $GITHUB_OUTPUT | |
| else | |
| if find . -name "*.java" -type f | grep -q .; then | |
| echo "found=true" >> $GITHUB_OUTPUT | |
| echo "source_path=." >> $GITHUB_OUTPUT | |
| else | |
| else |
| - name: Find OpenAPI spec | ||
| id: find-spec | ||
| run: | | ||
| spec=$(find . -type f \( -name "openapi*.yaml" -o -name "openapi*.json" -o -name "swagger*.yaml" -o -name "swagger*.json" \) | head -1) |
There was a problem hiding this comment.
This script only finds and uses the first OpenAPI/Swagger spec file using head -1. If the repository contains multiple API specification files, only the first one will be tested by EthicalCheck. This could leave other API specs unvalidated for security issues. Consider testing all spec files or making it explicit which one should be tested.
| cargo fmt -- --check || true | ||
| cargo clippy -- -D warnings || true |
There was a problem hiding this comment.
Rust linting commands have || true appended, which causes them to always succeed even when format violations or clippy warnings are found. This means the CI pipeline will pass even with code quality issues. If these checks are meant to be enforced, remove the || true suffixes. If they're meant to be informational only, this should be documented.
| cargo fmt -- --check || true | |
| cargo clippy -- -D warnings || true | |
| cargo fmt -- --check | |
| cargo clippy -- -D warnings |
| -Dsonar.projectKey=${{ vars.SONAR_PROJECT_KEY || 'FlexNetOS_noa_ark_os' }} | ||
| -Dsonar.organization=${{ vars.SONAR_ORGANIZATION || 'flexnetos' }} |
There was a problem hiding this comment.
Using fallback default values like 'FlexNetOS_noa_ark_os' and 'flexnetos' for SonarCloud configuration could lead to unexpected behavior. If these are not the actual project values and the variables aren't set, the workflow will attempt to use incorrect project keys, potentially causing failures or publishing results to the wrong project. Consider failing early if required configuration is missing rather than using arbitrary defaults.
| pnpm eslint . \ | ||
| --ext .js,.jsx,.ts,.tsx \ | ||
| --format @microsoft/eslint-formatter-sarif \ | ||
| --output-file eslint-results.sarif || true |
There was a problem hiding this comment.
Adding || true to the eslint command on line 59 combined with continue-on-error: true on line 60 creates redundant error suppression. The || true makes the command always succeed, so the continue-on-error setting becomes unnecessary. Choose one approach for consistency - either use || true in the command or rely on continue-on-error: true.
| --output-file eslint-results.sarif || true | |
| --output-file eslint-results.sarif |
| if [ -d repos/MicroAgentStack ]; then | ||
| cd repos/MicroAgentStack | ||
| flake8 . --count --select=E9,F63,F7,F82 --show-source --statistics || true | ||
| black --check . || true |
There was a problem hiding this comment.
Python linting commands have || true appended, which causes them to always succeed even when lint violations are found. This means the CI pipeline will pass even with code quality issues. If these checks are meant to be enforced, remove the || true suffixes. If they're meant to be informational only, this should be documented.
| black --check . || true | |
| black --check . |
| - name: Build Docker Image | ||
| if: hashFiles('Dockerfile') != '' |
There was a problem hiding this comment.
The Docker build step checks if a Dockerfile exists using hashFiles('Dockerfile') != '', but this only checks for a file named exactly 'Dockerfile' at the repository root. The workflow should either specify a clear path to the expected Dockerfile or handle cases where the Dockerfile might be in a subdirectory. Additionally, hashFiles returns an empty string if no files match, but also if there's an error, which could mask issues.
| - name: Build Docker Image | |
| if: hashFiles('Dockerfile') != '' | |
| - name: Ensure Dockerfile exists | |
| run: | | |
| if ! find . -type f -name 'Dockerfile' | grep -q .; then | |
| echo "ERROR: No Dockerfile found in repository (searched recursively)." | |
| exit 1 | |
| fi | |
| - name: Build Docker Image |
| - name: Check for Dockerfile | ||
| id: check-dockerfile | ||
| run: | | ||
| if [ -f "services/agent-registry/Dockerfile" ]; then | ||
| echo "dockerfile_exists=true" >> $GITHUB_OUTPUT | ||
| echo "dockerfile_path=services/agent-registry/Dockerfile" >> $GITHUB_OUTPUT |
There was a problem hiding this comment.
The workflow hardcodes a specific Dockerfile path 'services/agent-registry/Dockerfile'. This creates tight coupling between the workflow and a specific project structure. If the service name changes or the Dockerfile is moved, the workflow will silently skip the SBOM scan. Consider using a more flexible approach or making this path configurable through repository variables.
| - name: Check for Dockerfile | |
| id: check-dockerfile | |
| run: | | |
| if [ -f "services/agent-registry/Dockerfile" ]; then | |
| echo "dockerfile_exists=true" >> $GITHUB_OUTPUT | |
| echo "dockerfile_path=services/agent-registry/Dockerfile" >> $GITHUB_OUTPUT | |
| - name: Find Dockerfile | |
| id: check-dockerfile | |
| run: | | |
| DOCKERFILE_PATH=$(find services -type f -name 'Dockerfile' | head -n 1) | |
| if [ -n "$DOCKERFILE_PATH" ]; then | |
| echo "dockerfile_exists=true" >> $GITHUB_OUTPUT | |
| echo "dockerfile_path=$DOCKERFILE_PATH" >> $GITHUB_OUTPUT |
| uses: actions/setup-java@v4 | ||
| with: | ||
| java-version: '11' | ||
| java-version: '17' |
There was a problem hiding this comment.
The Java version has been upgraded from 11 to 17, but PMD configuration may need adjustment. Java 17 has different language features and syntax compared to Java 11. Verify that the PMD rulesets being used are compatible with Java 17 and that any Java code in the repository is actually using Java 17 features.
No description provided.