feat(build): enforce strict warnings across all linters#392
feat(build): enforce strict warnings across all linters#392WilliamBerryiii wants to merge 7 commits intomainfrom
Conversation
- add ShellCheck CI workflow, PowerShell wrapper, and .shellcheckrc config - promote YamlLint warnings to failures for strict warnings compliance - add ShellCheck and YamlLint Pester unit tests with strict mode coverage - integrate ShellCheck into pr-validation and main workflows - update CONTRIBUTING.md warning policy table and package.json scripts Closes #6 🔧 - Generated by Copilot
…e paths - remove LASTEXITCODE reset that silently masked all test failures in CI - add explicit exit 1 when Pester reports failed tests - fix test dot-source paths from broken scripts/lib symlinks to shared/lib 🐛 - Generated by Copilot
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Snapshot WarningsEnsure that dependencies are being submitted on PR branches and consider enabling retry-on-snapshot-warnings. See the documentation for more information and troubleshooting advice. OpenSSF Scorecard
Scanned Files
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #392 +/- ##
==========================================
+ Coverage 50.48% 50.86% +0.38%
==========================================
Files 267 268 +1
Lines 18188 18302 +114
Branches 1855 1903 +48
==========================================
+ Hits 9182 9310 +128
+ Misses 8716 8702 -14
Partials 290 290
*This pull request uses carry forward flags. Click here to find out more.
🚀 New features to boost your workflow:
|
- default exit code to failure before try block prevents undefined variable error under StrictMode when CLI binary is not installed 🐛 - Generated by Copilot
rezatnoMsirhC
left a comment
rezatnoMsirhC
left a comment
There was a problem hiding this comment.
I'm now realizing this PR is for linters and not tests, so leaving soft-fail: true on terraform-tests and go-tests was probably intentional. Do we have another issue for enforcing strict warnings/errors on those later on?
| name: Terraform Tests | ||
| uses: ./.github/workflows/terraform-tests.yml | ||
| with: | ||
| soft-fail: true |
There was a problem hiding this comment.
Shouldn't this be soft-fail: false?
There was a problem hiding this comment.
Correct — updated to soft-fail: false. After merging main, all linting jobs (Terraform Lint, Terraform Validation, Go Lint, ShellCheck) now use soft-fail: false. The new test jobs added by main (Terraform Tests, Go Tests) and Terraform Docs Check remain soft-fail: true as they're outside the scope of this PR.
There was a problem hiding this comment.
Correction on my earlier reply — this comment points to the Terraform Tests job, which intentionally stays soft-fail: true. Test jobs are out of scope for this PR. The linting jobs above (Terraform Lint, Terraform Validation) are the ones changed to soft-fail: false.
| name: Go Tests | ||
| uses: ./.github/workflows/go-tests.yml | ||
| with: | ||
| soft-fail: true |
There was a problem hiding this comment.
Shouldn't this be soft-fail: false?
There was a problem hiding this comment.
Yep — changed to soft-fail: false. Same as the Terraform Validation comment above, all linting jobs are now strict.
There was a problem hiding this comment.
Correction — this comment points to the Go Tests job, which intentionally stays soft-fail: true. The linting jobs (Go Lint, etc.) are the ones changed to soft-fail: false.
| name: Terraform Tests | ||
| uses: ./.github/workflows/terraform-tests.yml | ||
| with: | ||
| soft-fail: true |
There was a problem hiding this comment.
Shouldn't this be soft-fail: false?
There was a problem hiding this comment.
Fixed — now soft-fail: false. Same treatment as in main.yml: all linting jobs are strict, test jobs remain soft-fail.
There was a problem hiding this comment.
Correction — this comment points to the Terraform Tests job, which intentionally stays soft-fail: true. This PR only enforces strict failures on linting jobs (Terraform Validation, Go Lint, ShellCheck). Test jobs remain soft-fail to avoid blocking PRs on infrastructure test flakiness.
| name: Go Tests | ||
| uses: ./.github/workflows/go-tests.yml | ||
| with: | ||
| soft-fail: true |
There was a problem hiding this comment.
Shouldn't this be soft-fail: false?
There was a problem hiding this comment.
Done — changed to soft-fail: false.
There was a problem hiding this comment.
Correction — this comment points to the Go Tests job, which intentionally stays soft-fail: true. Same as above: only linting jobs are made strict in this PR.
| $excludeDirs = @('.venv', 'external', 'node_modules', '.git', 'docs/docusaurus') | ||
| $allShFiles = @(Get-ChildItem -Path $repoRoot -Filter '*.sh' -Recurse -File | Where-Object { | ||
| $relativePath = $_.FullName.Substring($repoRoot.Length + 1) | ||
| $excluded = $false | ||
| foreach ($dir in $excludeDirs) { | ||
| if ($relativePath -like "$dir*" -or $relativePath -like "*/$dir/*" -or $relativePath -like "*\$dir\*") { | ||
| $excluded = $true | ||
| break | ||
| } | ||
| } | ||
| -not $excluded | ||
| }) |
There was a problem hiding this comment.
I'm not sure if this will ever be a problem, but this exclusion pattern would detect false positives like external_tools/ if we ever add a directory like that to this repo (which we might want to spellcheck). I can't imagine the other entries in $excludeDirs causing any problems though.
There was a problem hiding this comment.
Good catch — tightened the exclusion matching. The filter now normalizes paths to forward slashes and uses -like "$dir/*" -or -like "*/$dir/*", which requires a directory boundary (start-of-path or /) before the excluded name. So external_tools/foo.sh would NOT match the external exclusion — only paths like external/foo.sh or some/external/foo.sh will match.
…t-warnings # Conflicts: # .github/workflows/main.yml # .github/workflows/pr-validation.yml # package.json # scripts/linting/Invoke-ShellCheck.ps1 # scripts/tests/lib/Get-VerifiedDownload.Tests.ps1 # scripts/tests/lib/terraform-outputs.Tests.ps1 # scripts/tests/linting/Invoke-ShellCheck.Tests.ps1
- update PSScriptAnalyzer path from shared/ci/linting/ to scripts/linting/ 📝 - Generated by Copilot
…k.ps1 - update relative path from ../../../scripts/lib/ to ../lib/ after shared/ci rename 🔧 - Generated by Copilot
4 participants
This PR established a warnings-as-errors policy across all CI linters and fixed a critical bug where Pester test failures were silently masked in CI. ShellCheck was added as a new linter with full CI integration, YamlLint was promoted to strict mode, and all remaining soft-fail linter jobs were switched to hard-fail.
Closes #6
Description
Critical Bug Fix
The Pester test workflow contained a
$global:LASTEXITCODE = 0reset that silently swallowed all test failures, allowing broken builds to pass CI. This was removed and replaced with explicitexit 1calls when Pester reports failures or throws exceptions. Two test files also had stale dot-source paths referencing thescripts/lib/symlink target instead of the canonicalshared/lib/location — these were corrected.ShellCheck Integration
A complete ShellCheck linting pipeline was added to the repository:
shell=bash,severity=warning, andexternal-sources=true.shfiles, runs ShellCheck with JSON output, classifies findings, writes structured results tologs/, and emits CI annotations — treating warnings as failuressoft-fail: falseYamlLint Strict Mode
Invoke-YamlLint.ps1 was updated to treat warnings as failures by broadening the failure condition to include
$warningCount -gt 0. The corresponding Pester tests in Invoke-YamlLint.Tests.ps1 were updated to reflect strict mode expectations.Soft-Fail Policy
Linting jobs enforce strict failures (
soft-fail: false):main.ymlpr-validation.ymlsoft-fail: falsesoft-fail: falsesoft-fail: falsesoft-fail: falsesoft-fail: falsesoft-fail: falseTest and documentation jobs remain advisory (
soft-fail: true) to avoid blocking PRs on infrastructure test flakiness or non-critical doc checks:main.ymlpr-validation.ymlsoft-fail: truesoft-fail: truesoft-fail: truesoft-fail: truesoft-fail: truesoft-fail: trueDocumentation and Tooling
lint:shandlint:pyscripts and updatedlint:allto include themType of Change
Component(s) Affected
infrastructure/terraform/prerequisites/- Azure subscription setupinfrastructure/terraform/- Terraform infrastructureinfrastructure/setup/- OSMO control plane / Helmworkflows/- Training and evaluation workflowstraining/- Training pipelines and scriptsdocs/- DocumentationTesting Performed
Documentation Impact
Bug Fix Checklist
Checklist