-
Notifications
You must be signed in to change notification settings - Fork 6
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
Add GH workflow to run lint checks daily as well as for every commit and PR. #14
Conversation
WalkthroughA new GitHub Actions workflow file named Changes
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
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.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
.github/workflows/lint.yml (3)
12-19
: Excellent job on permissions and concurrency settings.The "write" permission for contents is correctly set to allow cancelling in-progress jobs. The concurrency setup is well-configured to prevent redundant runs, improving efficiency.
Consider adding a comment explaining why the "write" permission is needed, to improve clarity for future maintainers:
permissions: # "write" permission is required to cancel in-progress jobs contents: "write"
27-34
: Good setup for repository checkout and Python.The checkout step correctly includes submodules, which is crucial if the project depends on them. Setting up Python is also handled appropriately.
Consider using a more recent version of Python, such as 3.9 or 3.10, unless there's a specific requirement for 3.8. You could also use a matrix strategy for Python versions to ensure compatibility across multiple versions:
strategy: matrix: os: ["macos-latest", "ubuntu-latest"] python-version: ["3.8", "3.9", "3.10"] runs-on: ${{ matrix.os }} steps: # ... - uses: "actions/setup-python@v5" with: python-version: ${{ matrix.python-version }}
36-44
: Good setup for tool installation and lint task execution.The installation of necessary tools and the execution of the lint task are well-structured. The conditional step for installing coreutils on macOS is a good practice to ensure cross-platform compatibility.
Consider the following improvements:
Specify a version for the task CLI tool to ensure consistency:
- name: "Install task" run: "npm install -g @go-task/[email protected]" # Replace with the desired versionAdd a step to cache npm dependencies to speed up future runs:
- uses: actions/cache@v3 with: path: ~/.npm key: ${{ runner.OS }}-node-${{ hashFiles('**/package-lock.json') }} restore-keys: | ${{ runner.OS }}-node-Consider adding a step to output the versions of installed tools for debugging purposes:
- name: "Output tool versions" run: | task --version python --version
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- .github/workflows/lint.yml (1 hunks)
🔇 Additional comments (2)
.github/workflows/lint.yml (2)
1-10
: LGTM! Comprehensive trigger configuration.The workflow name is concise and the trigger configuration is comprehensive. It covers all necessary scenarios for running lint checks, including pull requests, pushes, and a daily schedule. The daily run at 00:15 UTC is a good choice to avoid high load periods.
21-26
: Well-structured job setup with cross-platform support.The matrix strategy to run the lint job on both macOS and Ubuntu latest versions is excellent. This ensures cross-platform compatibility and catches any potential OS-specific issues early in the development process.
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.
LGTM.
I agree with the Rabbit on this one:
Consider adding a step to output the versions of installed tools for debugging purposes.
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.
The PR title is fine for the final commit message.
Description
This PR adds a workflow to run
task lint:check
daily as well as for every commit and PR. This is largely based on the workflows in y-scope/yscope-log-viewer and y-scope/log-surgeon.Validation performed
lint.yaml
and validated the lint workflow failed due to the violation.Summary by CodeRabbit
New Features
Improvements