-
Notifications
You must be signed in to change notification settings - Fork 262
Add fast-track CI for version file updates #17429
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.
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
base: master
Are you sure you want to change the base?
Conversation
Optimize PR workflow to skip expensive CI/CD steps when PRs only update version files. Version files are updated multiple times daily via automation, but full CI (site build, Cypress tests, deployment) is unnecessary for these simple text file changes. Changes: - Add detect-changes job to identify version-only PRs - Add fast-track-version-update job with lightweight validation (~2-3 min) - Update buildSite to run only for substantive changes - Update notify job to monitor both validation paths - Update BUILD-AND-DEPLOY.md documentation Benefits: - Reduces version update PR time from 10-15 min to 2-3 min - Fewer transient CI failures due to reduced test surface - Maintains critical validations (whitespace check, Hugo build) - Works automatically for all four version files Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Review of PR #17429: Add fast-track CI for version file updatesSummaryThis PR adds an optimization to the pull request workflow that creates a fast-track validation path for PRs that only modify version files. The implementation is technically sound with one minor documentation issue and a few suggestions for improvement. Issues FoundBUILD-AND-DEPLOY.mdLine 1005-1006: Inconsistent numbering The numbered list uses inconsistent numbering. All items should start with Line 1022: Inconsistent numbering Same issue continues: Line 1044: Inconsistent numbering Same issue: Suggestions for Improvement.github/workflows/pull-request.ymlLines 74-75: Consider making the conditional more readable The multi-line conditional is functional but could be clearer with explicit parentheses: if: |
github.event.pull_request.head.repo.full_name == github.repository &&
(needs.detect-changes.outputs.version-files-only == 'false')Lines 146-149: Notification logic could be simplified The notification condition is complex. Consider whether you need if: |
(startsWith(github.ref, 'refs/heads/release/') || github.ref == 'refs/heads/master' || (github.event_name == 'pull_request' && github.event.pull_request.user.login == 'pulumi-bot')) &&
(needs.buildSite.result == 'failure' || needs.fast-track-version-update.result == 'failure')The BUILD-AND-DEPLOY.mdLine 1018-1020: Consider adding security note Since version files are served directly and affect Hugo templates, consider adding a note about the security rationale for whitespace validation: - Validate no whitespace in version files (prevents template injection/rendering issues)Positive Observations
Verification Questions for Author
Next Steps: Please fix the numbered list formatting in BUILD-AND-DEPLOY.md. The other suggestions are optional improvements for your consideration. If you'd like me to review additional changes or help with any fixes, mention @claude. |
|
Your site preview for commit 04d46d2 is ready! 🎉 http://www-testing-pulumi-docs-origin-pr-17429-04d46d25.s3-website.us-west-2.amazonaws.com. |
|
Makes sense for
However, I'm wondering about PRs that update Similarly, the PRs that update |
|
Claude finished @CamSoper's task in 35s —— View job Verifying Workflow LogicChecking how the workflow handles multi-file PRs vs. version-only PRs. Todo List:
Verification ResultYour understanding is correct. The workflow will properly handle multi-file PRs by running the full build process. Here's how it works: The Logic (lines 34-41)The
The critical check at line 37: if [[ "${{ steps.filter.outputs.version-files }}" == "true" && "${{ steps.filter.outputs.other-files }}" == "false" ]]; thenThis requires BOTH conditions:
Why Multi-File PRs Get Full BuildsFor Justin's examples:
Only True Version-Only PRs Fast-TrackOnly PRs like #17372 (which touch only
The |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! Overall makes sense to me, but it'd be good to get as much logic out of the workflow files as we can, just 'cause YAML. Let me know what you think.
My only other concern is more of a meta-level worry in that the minute you add an easy way to put path expressions on a "fast track" path to main, people start using it, and then stuff breaks. But I'm also a worrier. 😄 (And I guess since this is the PR workflow, we'll have the push build as a backup, so probably fine.)
|
|
||
| - name: Validate version files | ||
| run: | | ||
| # Check for whitespace in version files |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My docs-ops recollection is still coming online, but I think this check is either not needed (since the hugo build will fail as it does today if latest-dev-version contains any whitespace), or we should be running it for all of these files, in which case it'd probably belong in a script, like in build-site.sh. What do you think?
Documentation ReviewI've reviewed this PR focusing on the workflow changes and documentation updates. Here's my feedback: ✅ StrengthsDocumentation Quality:
Technical Implementation:
📝 Minor IssuesBUILD-AND-DEPLOY.md:
Workflow File (.github/workflows/pull-request.yml):
🎯 Overall AssessmentThis is a well-executed optimization with appropriate documentation. The changes are:
The documentation updates in BUILD-AND-DEPLOY.md properly reflect the workflow changes, meeting the requirement from review-criteria.md: "If changes are made to build scripts, GitHub Actions workflows, the Makefile, or infrastructure code, verify that BUILD-AND-DEPLOY.md has been updated to reflect these changes." ✅ Approval RecommendationThis PR meets documentation quality standards. The minor suggestions above are optional improvements, not blockers. Need additional reviews or fixes? Mention @claude in a comment. |
|
Your site preview for commit 04d46d2 is ready! 🎉 http://www-testing-pulumi-docs-origin-pr-17429-04d46d25.s3-website.us-west-2.amazonaws.com. |
Problem
Version files (
static/latest-dev-version,static/latest-version, etc.) are updated multiple times daily via automated PRs. These PRs often get delayed or blocked by:Current workflow runs expensive checks that aren't necessary for version files:
Proposed Solution
This PR adds a fast-track validation path for PRs that only change version files:
static/latest-dev-versionstatic/latest-versionstatic/esc/latest-versionstatic/customer-managed-workflow-agent/latest-versionHow It Works
New
detect-changesjob:dorny/paths-filterto analyze changed filesversion-files-onlyflag (true/false)New
fast-track-version-updatejob (runs when version-files-only=true):make ensure)hugo --minify --quiet)Updated
buildSitejob:version-files-only=falseWhat's Validated
✅ No whitespace in version files (enforced by Hugo template)
✅ Hugo can build successfully
✅ Dependencies install correctly
What's Skipped (for version files only)
❌ Full site deployment to S3
❌ Cypress browser tests
❌ Search index generation
These are unnecessary because version files are plain text served directly and don't affect site rendering.
Benefits
Questions for Review
@justinvp @cnunciato - I'd like your feedback on this approach:
If this seems unnecessary or overly complex, I'm happy to abandon the approach. The current workflow does eventually succeed, it just takes longer and occasionally hits transient failures.
Testing Plan
If approved:
static/latest-dev-version