-
Notifications
You must be signed in to change notification settings - Fork 34
Convert workflow timestamp check from bash to JavaScript #2944
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
Conversation
- Created check_workflow_timestamp.cjs with fs-based timestamp comparison - Added embedded resource directive to js.go - Updated compiler_jobs.go to use JavaScript instead of inline bash - Added comprehensive test suite with vitest Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
| @@ -0,0 +1,9 @@ | |||
| --- | |||
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.
@copilot move to folder with other test workflows
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.
Moved test workflow to pkg/cli/workflows/ directory in commit 250d1df.
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.
Pull Request Overview
This PR refactors the workflow timestamp check from inline bash script to a dedicated JavaScript implementation that runs via actions/github-script. The change improves testability, maintainability, and consistency with other GitHub Actions scripts in the codebase.
Key changes:
- Replaced inline bash timestamp check with JavaScript implementation
- Added comprehensive unit tests for the timestamp check logic
- Added Go integration test to verify the JavaScript is properly embedded in compiled workflows
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| pkg/workflow/js/check_workflow_timestamp.cjs | New JavaScript implementation of workflow timestamp checking logic |
| pkg/workflow/js/check_workflow_timestamp.test.cjs | Comprehensive test suite for the timestamp check script |
| pkg/workflow/js.go | Added embed directive to include the new timestamp check script |
| pkg/workflow/compiler_jobs.go | Replaced inline bash with JavaScript script via actions/github-script |
| pkg/workflow/check_workflow_timestamp_js_test.go | Go integration test verifying JavaScript embedding |
| .github/workflows/test-timestamp-js.md | Test workflow file for manual verification |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| import { describe, it, expect, beforeEach, afterEach, vi } from "vitest"; | ||
| import fs from "fs"; | ||
| import path from "path"; | ||
| import os from "os"; |
Copilot
AI
Nov 1, 2025
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.
Test files using CommonJS (.cjs extension) should use require() instead of ES6 import syntax. While Vitest supports ES6 imports in .cjs files, using require() is more consistent with the CommonJS module system. All the script being tested (check_workflow_timestamp.cjs) uses require(), so the test should match this pattern for consistency.
| import { describe, it, expect, beforeEach, afterEach, vi } from "vitest"; | |
| import fs from "fs"; | |
| import path from "path"; | |
| import os from "os"; | |
| const { describe, it, expect, beforeEach, afterEach, vi } = require("vitest"); | |
| const fs = require("fs"); | |
| const path = require("path"); | |
| const os = require("os"); |
Moved test-timestamp-js.md from .github/workflows/ to pkg/cli/workflows/ to align with other test workflows. Added .gitignore entries to prevent the .github/workflows version from being tracked. Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Converting "check workflow timestamp" to JavaScript
This PR converts the inline bash script for checking workflow file timestamps into a dedicated JavaScript file, following the pattern established by other safety checks like
check_stop_time.cjs.Summary
This change improves code consistency and maintainability by:
Plan
check_workflow_timestamp.cjsJavaScript filejs.gocompiler_jobs.goto use the JavaScript scriptBenefits
-ntoperatorImplementation Details
The JavaScript implementation:
fsmodule to check file existence and get modification timesmtime.getTime()for precise comparisoncore.error()and adds to step summary viacore.summaryactions/github-script@v8action instead of inline bashChanges
pkg/workflow/js/check_workflow_timestamp.cjs- JavaScript implementation using Node.js fs modulepkg/workflow/js/check_workflow_timestamp.test.cjs- Comprehensive JavaScript test suite (11 test cases)pkg/workflow/check_workflow_timestamp_js_test.go- Go test to verify compilation outputpkg/cli/workflows/test-timestamp-js.md- Test workflow in correct locationpkg/workflow/js.go- Added embed directive for new scriptpkg/workflow/compiler_jobs.go- Replaced 15 lines of inline bash with 7 lines calling JavaScript.gitignore- Added entries to prevent tracking test files in .github/workflowsTest Coverage
JavaScript tests cover:
Go tests verify:
actions/github-script@instead ofrun: |Original prompt
Note
Custom agent used: shell-2-script
Extract inline bash scripts from Go compiler code into separate .sh files with embedded resources for improved maintainability, organization, and reusability
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.