Fixing the drive action #347
Conversation
WalkthroughThe pull request updates the workflow configurations in Changes
Assessment against linked issues
Possibly related PRs
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
.github/workflows/drive.yml (1)
24-26: Consider documenting the changesGiven that this fixes a specific issue (#346), it would be helpful to add a comment explaining why these changes were necessary, particularly regarding the environment variables and X11 setup.
Add a comment like:
- name: Run docker-compose run: | + # Fix for issue #346: Ensure proper user permissions and X11 access xhost +local: USERNAME=$(whoami) USER_UID=$(id -u) USER_GID=$(id -g) docker compose up --quiet-pull --exit-code-from agent
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- .github/workflows/drive.yml (1 hunks)
🧰 Additional context used
🔇 Additional comments (3)
.github/workflows/drive.yml (3)
26-26: Good improvements to docker compose commandThe addition of
--quiet-pulland--exit-code-from agentflags are good improvements:
--quiet-pullreduces log noise--exit-code-from agentensures proper error propagation from the agent container
26-26: Verify environment variable handling on self-hosted runnerWhile setting
USERNAME,USER_UID, andUSER_GIDis a good practice for container permissions, ensure these variables are consistently available and set correctly on your self-hosted runner.Let's verify the environment setup:
✅ Verification successful
Based on the shell script results, I can now provide a final response as there is sufficient evidence about how these environment variables are used in the codebase.
Environment variables are properly configured and utilized in the Docker setup
The codebase shows consistent and proper usage of
USERNAME,USER_UID, andUSER_GIDvariables across multiple Docker-related files:
- These variables are properly passed through in
build/docker-compose.dev.yamlandbuild/agent_service.yaml- The Dockerfiles (
DockerfileandDockerfile_Submission) properly handle these variables to:
- Create the user and group with correct permissions
- Set up sudo access
- Configure ownership of workspace directories
- Set up Python paths and dependencies
The environment variable handling in the GitHub workflow is correctly implemented and aligns with how these variables are used throughout the Docker configuration.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if docker-compose file uses these environment variables rg -A 5 'USERNAME|USER_UID|USER_GID' # Look for any user-related configurations in Dockerfile or compose files ast-grep --pattern 'USER|GROUP'Length of output: 15278
25-25:⚠️ Potential issueSecurity Warning: Avoid using
xhost +local:The
xhost +local:command disables access control for the X server, which is a security risk. Additionally, this command is typically unnecessary in a CI/CD environment where GUI applications shouldn't be running.Consider these alternatives:
- If X11 forwarding is required, use the more secure
xauthapproach- If this is for testing GUI applications, consider using a virtual framebuffer like Xvfb
Let's verify if X11 is actually needed:
There was a problem hiding this comment.
Thank you.
@JulianTrommer
Instead of just adding xhost +local, we could modifythe Carla-simulator container by extending its configuration to include the -RenderOffScreen option? I guess we still need to have x11 access for rviz?
You already suggested this change in #320
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (3)
.github/workflows/build.yml (2)
88-88: Add newline at end of fileAdd a newline character at the end of the file to follow YAML best practices.
Apply this change:
name: pr_id - path: pr/ + path: pr/ +🧰 Tools
🪛 yamllint
[error] 88-88: no new line character at the end of file
(new-line-at-end-of-file)
76-88: Consider workflow documentation and reusabilityThe new PR ID handling functionality could benefit from:
- Adding comments explaining the purpose and integration with drive.yml
- Consider moving PR ID logic to a reusable composite action if this pattern is used across multiple workflows
Example documentation to add above the PR ID steps:
# The following steps save the PR number as an artifact # This is used by the drive workflow to associate test results # with the correct pull request🧰 Tools
🪛 actionlint
80-80: shellcheck reported issue in this script: SC2086:info:2:6: Double quote to prevent globbing and word splitting
(shellcheck)
🪛 yamllint
[error] 88-88: no new line character at the end of file
(new-line-at-end-of-file)
.github/workflows/drive.yml (1)
Line range hint
1-13: Document environment variables and runner requirements.The workflow uses environment variables and requires a self-hosted runner with specific capabilities (Docker, X11), but these requirements aren't documented.
Add a comment block at the top of the file:
name: Evaluate agent +# Required environment setup: +# - Self-hosted runner with: +# - Docker and Docker Compose installed +# - X11 server configured +# Environment variables: +# - AGENT_VERSION: Version tag for the agent image (default: latest) +# - COMPOSE_FILE: Path to the Docker Compose file for CI/CD on: workflow_run:
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- .github/workflows/build.yml (1 hunks)
- .github/workflows/drive.yml (2 hunks)
🧰 Additional context used
🪛 actionlint
.github/workflows/build.yml
80-80: shellcheck reported issue in this script: SC2086:info:2:6: Double quote to prevent globbing and word splitting
(shellcheck)
🪛 yamllint
.github/workflows/build.yml
[error] 88-88: no new line character at the end of file
(new-line-at-end-of-file)
🔇 Additional comments (1)
.github/workflows/drive.yml (1)
48-50:⚠️ Potential issueSecurity concern: Unrestricted X server access.
The
xhost +local:command grants unrestricted access to the X server for all local users, which is a security risk. Consider using a more restrictive approach.Apply this diff to implement a more secure solution:
- xhost +local: - USERNAME=$(whoami) USER_UID=$(id -u) USER_GID=$(id -g) docker compose up --quiet-pull --exit-code-from agent + XSOCK=/tmp/.X11-unix + XAUTH=/tmp/.docker.xauth + touch $XAUTH + xauth nlist $DISPLAY | sed -e 's/^..../ffff/' | xauth -f $XAUTH nmerge - + USERNAME=$(whoami) USER_UID=$(id -u) USER_GID=$(id -g) \ + XAUTH=$XAUTH DISPLAY=$DISPLAY \ + docker compose up --quiet-pull --exit-code-from agentLet's verify if the Docker Compose file is configured to use these X11 settings:
| - name: Save PR ID | ||
| env: | ||
| PR_ID: ${{ github.event.number }} | ||
| run: | | ||
| mkdir -p ./pr | ||
| echo $PR_ID > ./pr/pr_id | ||
|
|
||
| - name: Upload PR ID | ||
| uses: actions/upload-artifact@v4 | ||
| with: | ||
| name: pr_id | ||
| path: pr/ |
There was a problem hiding this comment.
Add conditional and improve artifact handling
The PR ID steps should only run during pull request events, and the shell script can be improved for better reliability.
Apply these improvements:
- name: Save PR ID
+ if: github.event_name == 'pull_request'
env:
PR_ID: ${{ github.event.number }}
run: |
mkdir -p ./pr
- echo $PR_ID > ./pr/pr_id
+ echo "$PR_ID" > ./pr/pr_id
- name: Upload PR ID
+ if: github.event_name == 'pull_request'
uses: actions/upload-artifact@v4
with:
name: pr_id
path: pr/
+ retention-days: 1Changes:
- Added
ifconditions to only run during pull requests - Added quotes around
$PR_IDto prevent word splitting - Set retention period to 1 day since this is temporary data
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - name: Save PR ID | |
| env: | |
| PR_ID: ${{ github.event.number }} | |
| run: | | |
| mkdir -p ./pr | |
| echo $PR_ID > ./pr/pr_id | |
| - name: Upload PR ID | |
| uses: actions/upload-artifact@v4 | |
| with: | |
| name: pr_id | |
| path: pr/ | |
| - name: Save PR ID | |
| if: github.event_name == 'pull_request' | |
| env: | |
| PR_ID: ${{ github.event.number }} | |
| run: | | |
| mkdir -p ./pr | |
| echo "$PR_ID" > ./pr/pr_id | |
| - name: Upload PR ID | |
| if: github.event_name == 'pull_request' | |
| uses: actions/upload-artifact@v4 | |
| with: | |
| name: pr_id | |
| path: pr/ | |
| retention-days: 1 |
🧰 Tools
🪛 actionlint
80-80: shellcheck reported issue in this script: SC2086:info:2:6: Double quote to prevent globbing and word splitting
(shellcheck)
🪛 yamllint
[error] 88-88: no new line character at the end of file
(new-line-at-end-of-file)
| let issue_number = Number(fs.readFileSync('./pr_id')); | ||
| github.rest.issues.createComment({ | ||
| issue_number: context.issue.number, | ||
| issue_number: issue_number, |
There was a problem hiding this comment.
Add validation for PR ID.
The PR ID is read from a file without validation, which could lead to errors if the file is empty or contains invalid data.
Apply this diff to add proper validation:
- let issue_number = Number(fs.readFileSync('./pr_id'));
+ const prIdContent = fs.readFileSync('./pr_id', 'utf8').trim();
+ const issue_number = Number(prIdContent);
+ if (!prIdContent || isNaN(issue_number)) {
+ core.setFailed(`Invalid PR ID: ${prIdContent}`);
+ return;
+ }
github.rest.issues.createComment({
issue_number: issue_number,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let issue_number = Number(fs.readFileSync('./pr_id')); | |
| github.rest.issues.createComment({ | |
| issue_number: context.issue.number, | |
| issue_number: issue_number, | |
| const prIdContent = fs.readFileSync('./pr_id', 'utf8').trim(); | |
| const issue_number = Number(prIdContent); | |
| if (!prIdContent || isNaN(issue_number)) { | |
| core.setFailed(`Invalid PR ID: ${prIdContent}`); | |
| return; | |
| } | |
| github.rest.issues.createComment({ | |
| issue_number: issue_number, |
| - name: 'Download artifact (PR ID)' | ||
| uses: actions/github-script@v6 | ||
| with: | ||
| script: | | ||
| let allArtifacts = await github.rest.actions.listWorkflowRunArtifacts({ | ||
| owner: context.repo.owner, | ||
| repo: context.repo.repo, | ||
| run_id: context.payload.workflow_run.id, | ||
| }); | ||
| let matchArtifact = allArtifacts.data.artifacts.filter((artifact) => { | ||
| return artifact.name == "pr_id" | ||
| })[0]; | ||
| let download = await github.rest.actions.downloadArtifact({ | ||
| owner: context.repo.owner, | ||
| repo: context.repo.repo, | ||
| artifact_id: matchArtifact.id, | ||
| archive_format: 'zip', | ||
| }); | ||
| let fs = require('fs'); | ||
| fs.writeFileSync(`${process.env.GITHUB_WORKSPACE}/pr_id.zip`, Buffer.from(download.data)); | ||
|
|
||
| - name: 'Unzip artifact (PR ID)' | ||
| run: unzip pr_id.zip |
There was a problem hiding this comment.
Add error handling for artifact operations.
The artifact download/unzip steps lack error handling for cases where the artifact might not exist or the download fails.
Apply this diff to add proper error handling:
- name: 'Download artifact (PR ID)'
uses: actions/github-script@v6
with:
script: |
let allArtifacts = await github.rest.actions.listWorkflowRunArtifacts({
owner: context.repo.owner,
repo: context.repo.repo,
run_id: context.payload.workflow_run.id,
});
let matchArtifact = allArtifacts.data.artifacts.filter((artifact) => {
return artifact.name == "pr_id"
})[0];
+ if (!matchArtifact) {
+ core.setFailed('No pr_id artifact found from the build workflow');
+ return;
+ }
let download = await github.rest.actions.downloadArtifact({
owner: context.repo.owner,
repo: context.repo.repo,
artifact_id: matchArtifact.id,
archive_format: 'zip',
});
let fs = require('fs');
fs.writeFileSync(`${process.env.GITHUB_WORKSPACE}/pr_id.zip`, Buffer.from(download.data));
- name: 'Unzip artifact (PR ID)'
- run: unzip pr_id.zip
+ run: |
+ if [ ! -f pr_id.zip ]; then
+ echo "pr_id.zip not found"
+ exit 1
+ fi
+ unzip pr_id.zip📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - name: 'Download artifact (PR ID)' | |
| uses: actions/github-script@v6 | |
| with: | |
| script: | | |
| let allArtifacts = await github.rest.actions.listWorkflowRunArtifacts({ | |
| owner: context.repo.owner, | |
| repo: context.repo.repo, | |
| run_id: context.payload.workflow_run.id, | |
| }); | |
| let matchArtifact = allArtifacts.data.artifacts.filter((artifact) => { | |
| return artifact.name == "pr_id" | |
| })[0]; | |
| let download = await github.rest.actions.downloadArtifact({ | |
| owner: context.repo.owner, | |
| repo: context.repo.repo, | |
| artifact_id: matchArtifact.id, | |
| archive_format: 'zip', | |
| }); | |
| let fs = require('fs'); | |
| fs.writeFileSync(`${process.env.GITHUB_WORKSPACE}/pr_id.zip`, Buffer.from(download.data)); | |
| - name: 'Unzip artifact (PR ID)' | |
| run: unzip pr_id.zip | |
| - name: 'Download artifact (PR ID)' | |
| uses: actions/github-script@v6 | |
| with: | |
| script: | | |
| let allArtifacts = await github.rest.actions.listWorkflowRunArtifacts({ | |
| owner: context.repo.owner, | |
| repo: context.repo.repo, | |
| run_id: context.payload.workflow_run.id, | |
| }); | |
| let matchArtifact = allArtifacts.data.artifacts.filter((artifact) => { | |
| return artifact.name == "pr_id" | |
| })[0]; | |
| if (!matchArtifact) { | |
| core.setFailed('No pr_id artifact found from the build workflow'); | |
| return; | |
| } | |
| let download = await github.rest.actions.downloadArtifact({ | |
| owner: context.repo.owner, | |
| repo: context.repo.repo, | |
| artifact_id: matchArtifact.id, | |
| archive_format: 'zip', | |
| }); | |
| let fs = require('fs'); | |
| fs.writeFileSync(`${process.env.GITHUB_WORKSPACE}/pr_id.zip`, Buffer.from(download.data)); | |
| - name: 'Unzip artifact (PR ID)' | |
| run: | | |
| if [ ! -f pr_id.zip ]; then | |
| echo "pr_id.zip not found" | |
| exit 1 | |
| fi | |
| unzip pr_id.zip |
Description
The drive action currently fails. This PR tries to fix the action.
Fixes #346
Type of change
Does this PR introduce a breaking change?
No.
Most important changes
Changes to the drive action
drive.yml.Checklist:
Summary by CodeRabbit