Skip to content
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.

Sign up for GitHub

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

Fixing the drive action #347

Merged
merged 2 commits into from
Oct 24, 2024
Merged

Fixing the drive action #347

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f01db34
First attempt at fixing the drive action
JulianTrommer Oct 23, 2024
5aa49df
Fixed simulation results comment
JulianTrommer Oct 24, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 13 additions & 0 deletions .github/workflows/build.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,3 +73,16 @@ jobs:
USERNAME=paf
USER_UID=1000
USER_GID=1000

- 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/
Comment on lines +77 to +88
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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: 1

Changes:

  1. Added if conditions to only run during pull requests
  2. Added quotes around $PR_ID to prevent word splitting
  3. 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.

Suggested change
- 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)

31 changes: 29 additions & 2 deletions .github/workflows/drive.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,34 @@ jobs:
run: |
echo "AGENT_VERSION=${AGENT_VERSION}"
echo "COMPOSE_FILE=${COMPOSE_FILE}"
- 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
Comment on lines +23 to +45
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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.

Suggested change
- 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


- name: Run docker-compose
run: docker compose up --quiet-pull --exit-code-from agent
run: |
xhost +local:
USERNAME=$(whoami) USER_UID=$(id -u) USER_GID=$(id -g) docker compose up --quiet-pull --exit-code-from agent
- name: Copy results
run: docker compose cp agent:/tmp/simulation_results.json .
- name: Stop docker-compose
Expand Down Expand Up @@ -50,8 +76,9 @@ jobs:
// add everything to the resultsTable
resultsTable = resultsTableHeader + '\n' + resultsTableDivider + '\n' + resultsTable.join('\n');
// add the results as a comment to the pull request
let issue_number = Number(fs.readFileSync('./pr_id'));
github.rest.issues.createComment({
issue_number: context.issue.number,
issue_number: issue_number,
Comment on lines +79 to +81
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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.

Suggested change
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,

owner: context.repo.owner,
repo: context.repo.repo,
body: "## Simulation results\n" + resultsTable
Expand Down
Loading