testing new db approach on CI #402
Conversation
|
Warning Rate limit exceeded@arkanoider has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 1 minutes and 28 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (2)
WalkthroughThe pull request introduces modifications to the GitHub Actions workflow for Rust and the Changes
Poem
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 (
|
arkanoider
force-pushed
the
ci-improve-db
branch
2 times, most recently
from
9c15dff to
6beea3d
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
.github/workflows/rust.yml (3)
14-14: Consider using absolute path for DATABASE_URLWhile the SQLite URL format is correct, using a relative path (
./mostro.db) in CI environment could be fragile. Consider using an absolute path to ensure consistent behavior.- DATABASE_URL: sqlite://./mostro.db + DATABASE_URL: sqlite://${{ github.workspace }}/mostro.db
30-40: Optimize branch selection logicThe current implementation has duplicate checkout steps that could be simplified into a single step using a dynamic reference.
- - name: Select branch PR - if: ${{ github.ref != 'refs/heads/main' }} - uses: actions/checkout@v4 - with: - ref: ${{ github.event.pull_request.head.ref }} - - - name: Select branch main - if: ${{ github.ref == 'refs/heads/main' }} - uses: actions/checkout@v4 - with: - ref: main + - name: Checkout code + uses: actions/checkout@v4 + with: + ref: ${{ github.event_name == 'pull_request' && github.event.pull_request.head.ref || 'main' }}🧰 Tools
🪛 yamllint (1.35.1)
[error] 32-32: trailing spaces
(trailing-spaces)
[error] 35-35: trailing spaces
(trailing-spaces)
[error] 38-38: trailing spaces
(trailing-spaces)
24-24: Fix YAML formatting issuesRemove trailing spaces from lines 24, 25, 29, 32, 35, and 38 to maintain consistent formatting.
Also applies to: 25-25, 29-29, 32-32, 35-35, 38-38
🧰 Tools
🪛 yamllint (1.35.1)
[error] 24-24: trailing spaces
(trailing-spaces)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
mostro.dbis excluded by!**/*.db
📒 Files selected for processing (2)
.github/workflows/rust.yml(1 hunks).gitignore(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- .gitignore
🧰 Additional context used
🪛 yamllint (1.35.1)
.github/workflows/rust.yml
[error] 24-24: trailing spaces
(trailing-spaces)
[error] 25-25: trailing spaces
(trailing-spaces)
[error] 29-29: trailing spaces
(trailing-spaces)
[error] 32-32: trailing spaces
(trailing-spaces)
[error] 35-35: trailing spaces
(trailing-spaces)
[error] 38-38: trailing spaces
(trailing-spaces)
🔇 Additional comments (1)
.github/workflows/rust.yml (1)
20-23: Review security implications of broad write permissions
While write permissions are needed for committing database files, granting broad contents: write access might be more permissive than necessary. Consider if a more restricted scope would suffice.
✅ Verification successful
Write permissions are appropriately scoped for the workflow's needs
The contents: write permission is justified here as the workflow specifically needs to commit two files to the repository:
mostro.db(SQLite database file)sqlx-data.json(SQLx offline mode data)
These commits are done through the EndBug/add-and-commit action with explicit file paths, which provides a controlled way of modifying repository contents. The broad write permission is necessary for this operation but the actual usage is well-constrained by the action's configuration.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if the workflow modifies files outside of expected paths
gh api graphql -f query='
query($owner:String!, $name:String!, $branch:String!) {
repository(owner:$owner, name:$name) {
object(expression:$branch) {
... on Commit {
history(first:10) {
nodes {
author {
name
}
messageHeadline
additions {
path
}
}
}
}
}
}
}' -F owner='MostroP2P' -F name='mostro' -F branch='main' | \
jq '.data.repository.object.history.nodes[] | select(.author.name=="GitHub Actions")'
Length of output: 702
Script:
#!/bin/bash
# Let's check the workflow file to understand what files it actually modifies
cat .github/workflows/rust.yml
# Also check for any other workflow files that might provide context
fd -e yml -e yaml . .github/workflows/
Length of output: 1749
arkanoider
force-pushed
the
ci-improve-db
branch
from
5a40afc to
f41dd3b
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
.github/workflows/rust.yml (1)
60-69:⚠️ Potential issueRemove force flag from file additions
Using the --force flag could potentially cause issues with local development.
Apply this change to prevent potential conflicts:
add: | - - mostro.db --force - - sqlx-data.json --force + - mostro.db + - sqlx-data.json
🧹 Nitpick comments (3)
.github/workflows/rust.yml (3)
30-40: Simplify branch selection logicThe current implementation uses two separate checkout steps, which adds unnecessary complexity.
Consider simplifying to a single checkout step:
- - name: Select branch PR - if: ${{ github.ref != 'refs/heads/main' }} - uses: actions/checkout@v4 - with: - ref: ${{ github.event.pull_request.head.ref }} - - - name: Select branch main - if: ${{ github.ref == 'refs/heads/main' }} - uses: actions/checkout@v4 - with: - ref: main + - name: Checkout + uses: actions/checkout@v4 + with: + ref: ${{ github.event_name == 'pull_request' && github.event.pull_request.head.ref || 'main' }}🧰 Tools
🪛 yamllint (1.35.1)
[error] 32-32: trailing spaces
(trailing-spaces)
[error] 35-35: trailing spaces
(trailing-spaces)
[error] 38-38: trailing spaces
(trailing-spaces)
42-43: Consider using version range for sqlx-cliWhile version pinning provides stability, it might prevent getting important updates and security fixes.
Consider using a version range:
- run: cargo install sqlx-cli --version 0.6.2 + run: cargo install sqlx-cli --version "^0.6.2"
60-69: Consider optimizing commit step to avoid unnecessary commitsThe current implementation will create commits even when there are no changes to the database files.
Consider adding a check for changes before committing:
- name: Commit mostro.db and sqlx-data.json + run: | + if [[ -n "$(git status --porcelain mostro.db sqlx-data.json)" ]]; then uses: EndBug/add-and-commit@v9 with: default_author: github_actions message: 'chore(db): Update mostro.db and sqlx-data.json [skip ci]' committer_name: GitHub Actions committer_email: [email protected] add: | - mostro.db - sqlx-data.json + fi
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/rust.yml(1 hunks)
🧰 Additional context used
🪛 yamllint (1.35.1)
.github/workflows/rust.yml
[error] 24-24: trailing spaces
(trailing-spaces)
[error] 25-25: trailing spaces
(trailing-spaces)
[error] 29-29: trailing spaces
(trailing-spaces)
[error] 32-32: trailing spaces
(trailing-spaces)
[error] 35-35: trailing spaces
(trailing-spaces)
[error] 38-38: trailing spaces
(trailing-spaces)
[error] 45-45: trailing spaces
(trailing-spaces)
[error] 46-46: trailing spaces
(trailing-spaces)
[error] 47-47: trailing spaces
(trailing-spaces)
[error] 51-51: trailing spaces
(trailing-spaces)
[error] 52-52: trailing spaces
(trailing-spaces)
[error] 53-53: trailing spaces
(trailing-spaces)
🔇 Additional comments (2)
.github/workflows/rust.yml (2)
14-14: LGTM: Environment and permissions are properly configured
The DATABASE_URL and write permissions are correctly set up to support the automated database management workflow.
Also applies to: 20-23
55-58: LGTM: SQLx preparation steps are well-implemented
The preparation and verification of the offline file are properly configured to support offline compilation.
arkanoider
force-pushed
the
ci-improve-db
branch
from
de4d063 to
dc4b295
Compare
|
If I understand this correctly, with every new commit a new commit on mostrod repository will be generated automatically? |
You're right! Was not able to do it better,but now:
Don't know if you like it, I see some advanteges, also if not a perfect solution. |
Hi @grunch , @Catrya , @bilthon
being in office today and so a bit more in trouble to work on mostro testing, i tweaked a bit for the
mostro.db"issue".I mean... we also been there...time passes me or @grunch change something in sql files and the you pull latest work try to create an order...take it...and boom db error!! Why?! WTF! It's always the same ( @Catrya knows well 😄 ).
So you need to run
init_db.shand copy your new db in mostro folder.I was trying to use build scritp (
build.rs) to improve this and automatically generate at every build the correct db, but murphy's law hit me and I run into this so called deadlock issue:https://lpalmieri.com/posts/cargo-px/#invoking-cargo
and i admit I was not able to exit from there easily.
Ok! After this what i am proposing here is to generate:
from github actions. Not was super immediate, but I think it's working now. Some details:
init_db.shfor normal user at least he wants to change locally sql files in migrations folder..ymlfile in mostro will do all the job thatinit_dbdid locally but in the github world creating the file for us.The downside is a commit more generated by the actions, not a big issue imo, but i am here to listen your opinion.
Take a look!
Summary by CodeRabbit
New Features
DATABASE_URLfor database configuration.Bug Fixes
.gitignoreto track variations ofmostro.dbwhile ignoring the specific file.sqlx-data.jsonin.gitignore.Chores