Skip to content

chore(GHA): add a GHA to auto update R data#34

Merged
Bai-Li-NOAA merged 2 commits into
mainfrom
add-gha-update-data
Jun 15, 2026
Merged

chore(GHA): add a GHA to auto update R data#34
Bai-Li-NOAA merged 2 commits into
mainfrom
add-gha-update-data

Conversation

@Bai-Li-NOAA

Copy link
Copy Markdown
Collaborator

What is the feature?

  • add a GHA to auto update R data

How have you implemented the solution?

  • develop a reusable workflow update-data-r.yml
    for updating data files in the package when
    changes are made to the /data-raw

Does the PR impact any other area of the project, maybe another repo?

  • No

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@Bai-Li-NOAA

Copy link
Copy Markdown
Collaborator Author

code review from copilot:
Issues and Bugs Found
🔴 Critical Issue: Checkout Step Logic Error
Location: Lines 43-52

There's a fundamental problem with the checkout logic. Both conditional checkout steps will never execute correctly:

YAML

  • name: checkout, make changes and submit as pr on new branch
    if: inputs.commit-directly == false
    uses: actions/checkout@v4

  • name: checkout, make changes directly
    if: inputs.commit-directly == true
    uses: actions/checkout@v4
    with:
    ref: ${{ github.head_ref }}
    fetch-depth: 0
    Problem: The first checkout (line 43-45) has a condition but no git configuration. The second checkout (line 47-52) always uses ref: ${{ github.head_ref }}, which is only populated in pull request contexts, not on direct pushes.

Impact:

When commit-directly: true (normal case), github.head_ref will be empty/null on push events, causing checkout to fail
The workflow won't work as designed for the typical use case
Fix: Consolidate the checkout logic:

YAML

  • name: checkout repository
    uses: actions/checkout@v4
    with:
    ref: ${{ inputs.commit-directly == true && github.ref_name || github.head_ref || github.ref_name }}
    fetch-depth: 0
    🟡 High Priority: R Script Execution Vulnerability
    Location: Line 70

R
status <- system(paste("Rscript ", shQuote(script)))
Problem: While shQuote() provides some protection, the system() function with shell=TRUE can still be risky if script filenames contain special characters or if devtools::install() modifies the environment unexpectedly.

Recommendation: Use system2() instead for safer command execution:

R
status <- system2("Rscript", script)
🟡 Issue: Missing Error Handling for devtools::install()
Location: Line 65

R
devtools::install()
Problem: If devtools::install() fails, the workflow continues and attempts to execute R scripts. This could leave the environment in an inconsistent state.

Fix: Add error checking:

R
if (!devtools::install()) stop("Failed to install package")
🟡 Issue: Conditional Step Dependency
Location: Lines 54-61 (R setup steps)

Problem: The R environment setup steps (r-lib/actions) execute regardless of which checkout condition was met. While this works, they should ideally run after the appropriate checkout to avoid redundant dependency checks.

Recommendation: Add conditional execution to R setup steps:

YAML

  • uses: r-lib/actions/setup-r@v2
    if: ${{ steps.checkout-step.outcome == 'success' }} # Reference appropriate step
    🟠 Minor Issue: Inconsistent Spacing in YAML
    Location: Line 9 (indentation)

YAML
on:
workflow_call: # Should be two spaces, is one space
This should be:

YAML
on:
workflow_call:
🟠 Minor Issue: Indentation in Create PR Body
Location: Line 107

YAML
body: |
Auto-generated by [update-data-r.yml][1] # Extra leading space
Should be:

YAML
body: |
Auto-generated by [update-data-r.yml][1]
Merge Readiness Assessment
Status: ⚠️ Not Ready to Merge

Blockers:

Critical checkout logic bug — The workflow won't function in the primary use case (direct commit mode)
Missing error handling — R script failures could be silently missed
Recommendations before merge:

Fix the checkout condition logic
Add error handling to devtools::install()
Test both commit-directly: true and false modes
Verify the workflow runs successfully on a test branch with changes to /data-raw/

@Bai-Li-NOAA Bai-Li-NOAA force-pushed the add-gha-update-data branch from 86c5b17 to b16a66a Compare June 15, 2026 21:05
@github-actions

Copy link
Copy Markdown

Code Metrics Report

Coverage Code to Test Ratio Test Execution Time
87.1% 1:1.2 16s

Reported by octocov

@Bai-Li-NOAA

Copy link
Copy Markdown
Collaborator Author

Fixed the checkout logic bug and system() call. Left the rest as they are because R throws a hard error on installation failure and GHA naturally aborts sequential steps on failure.

@Bai-Li-NOAA Bai-Li-NOAA merged commit 62085fc into main Jun 15, 2026
11 checks passed
@github-project-automation github-project-automation Bot moved this from In progress to Done in Ecosystem coding for assessments Jun 15, 2026
@Bai-Li-NOAA Bai-Li-NOAA deleted the add-gha-update-data branch June 15, 2026 21:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Development

Successfully merging this pull request may close these issues.

Set up GHA to detect raw data changes

1 participant