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.

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

Work CI-CD #1198

Merged
Merged

Conversation

networkfusion
Copy link
Member

@networkfusion networkfusion commented Dec 16, 2024

Description

  • Improve update-dependencies timeschedule.
  • Use a better time schedule for the cron.

Motivation and Context

The current timeschedule is less likely to have support from core team members.
Linked to nanoframework/Home#1565

How Has This Been Tested?

Screenshots

Types of changes

  • Improvement (non-breaking change that improves a feature, code or algorithm)
  • Bug fix (non-breaking change which fixes an issue with code or algorithm)
  • New feature (non-breaking change which adds functionality to code)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Config and build (change in the configuration and build system, has no impact on code or features)
  • Dependencies (update dependencies and changes associated, has no impact on code or features)
  • Unit Tests (add new Unit Test(s) or improved existing one(s), has no impact on code or features)
  • Documentation (changes or updates in the documentation, has no impact on code or features)

Checklist:

  • My code follows the code style of this project (only if there are changes in source code).
  • My changes require an update to the documentation (there are changes that require the docs website to be updated).
  • I have updated the documentation accordingly (the changes require an update on the docs in this repo).
  • I have read the CONTRIBUTING document.
  • I have tested everything locally and all new and existing tests passed (only if there are changes in source code).
  • I have added new tests to cover my changes.

Summary by CodeRabbit

Summary by CodeRabbit

  • Chores
    • Updated scheduling for dependency update workflows to run at new times on Mondays and Wednesdays.
    • Reduced timeout for dependency update jobs from 180 minutes to 15 minutes across multiple workflows.
    • Adjusted timeout for one workflow to 120 minutes, considering additional processing time.

Use a better time schedule
Copy link

coderabbitai bot commented Dec 16, 2024

Walkthrough

The pull request modifies five GitHub workflow files (.github/workflows/update-dependencies-*.yml) responsible for updating .NET nanoFramework dependencies. The key changes across these workflow files include shifting the cron schedule from Wednesdays and Fridays to Mondays and Wednesdays, adjusting the execution times to between 02:00 and 06:30 UTC, and significantly reducing the job timeout from 180 minutes to either 15 or 120 minutes. These modifications standardize the dependency update process across multiple workflow configurations.

Changes

File Change Summary
.github/workflows/update-dependencies-1.yml - Cron schedule changed from 00:00 UTC Wed/Fri to 02:00 UTC Mon/Wed
- Job timeout reduced from 180 to 15 minutes
.github/workflows/update-dependencies-2.yml - Cron schedule changed from 00:15 UTC Wed/Fri to 02:30 UTC Mon/Wed
- Job timeout reduced from 180 to 15 minutes
.github/workflows/update-dependencies-3.yml - Cron schedule changed from 00:30 UTC Wed/Fri to 03:00 UTC Mon/Wed
- Job timeout reduced from 180 to 15 minutes
.github/workflows/update-dependencies-4.yml - Cron schedule changed from 01:45 UTC Wed/Fri to 03:30 UTC Mon/Wed
- Job timeout adjusted from 180 to 120 minutes
.github/workflows/update-dependencies-5.yml - Cron schedule changed from 04:30 UTC Wed/Fri to 06:30 UTC Mon/Wed
- Job timeout reduced from 180 to 15 minutes

Possibly related PRs

  • Update 2 NuGet dependencies #1163: This PR updates the nanoFramework.Iot.Device.Ft6xx6x and nanoFramework.M5Core2 dependencies, which are relevant to the .NET nanoFramework dependencies being updated in the main PR.
  • Update 2 NuGet dependencies #1170: This PR updates the nanoFramework.Iot.Device.Common.GnssDevice and nanoFramework.Iot.Device.Ws28xx dependencies, also related to the .NET nanoFramework dependencies.
  • Update 2 NuGet dependencies #1180: This PR updates the nanoFramework.Hardware.Esp32 and nanoFramework.M5Core2 dependencies, which are part of the same ecosystem of .NET nanoFramework dependencies being addressed in the main PR.

Suggested labels

CI: Publish Release


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?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR. (Beta)
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link

@coderabbitai coderabbitai bot left a 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

🧹 Nitpick comments (3)
.github/workflows/update-dependencies-3.yml (1)

10-12: Improve comment clarity regarding workflow sequencing

The current comment about timing is somewhat vague. Consider being more explicit about the workflow sequence.

-    # Set 15 min offset from round two to allow enough time for any changes to propagate.
+    # Round 3/5: Executes 15 minutes after round 2 (02:30 UTC)
🧰 Tools
🪛 yamllint (1.35.1)

[warning] 12-12: too many spaces after colon

(colons)

.github/workflows/update-dependencies-2.yml (1)

Line range hint 1-1: Consider documenting the dependency update strategy

The workflows form a coordinated system for updating dependencies, but the overall strategy (timing, sequencing, solution grouping) isn't documented. Consider adding a README.md in the .github/workflows directory explaining:

  1. The purpose and sequence of the 5 rounds
  2. The rationale for the Mon,Wed schedule
  3. The solution grouping strategy
  4. The timeout considerations and monitoring approach

I can help draft this documentation if you'd like.

🧰 Tools
🪛 yamllint (1.35.1)

[warning] 12-12: too many spaces after colon

(colons)

.github/workflows/update-dependencies-1.yml (1)

12-12: Fix YAML formatting: Remove extra spaces after colon in cron line

The YAML linter detected extra spaces after the colon.

-    - cron:  '00 02 * * Mon,Wed'
+    - cron: '00 02 * * Mon,Wed'
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 659df3d and 6fdbaed.

📒 Files selected for processing (5)
  • .github/workflows/update-dependencies-1.yml (2 hunks)
  • .github/workflows/update-dependencies-2.yml (2 hunks)
  • .github/workflows/update-dependencies-3.yml (2 hunks)
  • .github/workflows/update-dependencies-4.yml (1 hunks)
  • .github/workflows/update-dependencies-5.yml (2 hunks)
🧰 Additional context used
🪛 yamllint (1.35.1)
.github/workflows/update-dependencies-3.yml

[warning] 12-12: too many spaces after colon

(colons)

.github/workflows/update-dependencies-1.yml

[warning] 11-11: too many spaces after colon

(colons)

.github/workflows/update-dependencies-2.yml

[warning] 12-12: too many spaces after colon

(colons)

.github/workflows/update-dependencies-5.yml

[warning] 12-12: too many spaces after colon

(colons)

🔇 Additional comments (6)
.github/workflows/update-dependencies-5.yml (2)

10-12: Verify the 3-hour gap assumption

The comment indicates a 3-hour gap from round four, but we should verify this timing across all workflow files to ensure proper sequencing.

✅ Verification successful

Schedule gap between rounds 4 and 5 is correct

The verification confirms that round 4 runs at 02:45 UTC and round 5 runs at 05:45 UTC, which is indeed a 3-hour gap as stated in the comment. The complete schedule shows proper sequencing:

  • Round 1: 02:00 UTC
  • Round 2: 02:15 UTC
  • Round 3: 02:30 UTC
  • Round 4: 02:45 UTC
  • Round 5: 05:45 UTC
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check all update-dependencies workflow schedules
echo "Checking all workflow schedules:"
for i in {1..5}; do
  echo -n "Round $i: "
  grep -A 2 "cron:" .github/workflows/update-dependencies-$i.yml
done

Length of output: 1064

🧰 Tools
🪛 yamllint (1.35.1)

[warning] 12-12: too many spaces after colon

(colons)


23-23: Consider the implications of the reduced timeout

The timeout has been significantly reduced from 180 to 15 minutes. While the comment suggests confidence in the completion time, we should verify historical run times to ensure this is realistic.

Consider adding monitoring or alerts for jobs that approach or exceed the new timeout limit to help validate this change.

✅ Verification successful

Reducing timeout to 15 minutes is safe and reasonable

Based on the workflow execution history, the job consistently completes well under 15 minutes:

  • Average runtime is around 4-7 minutes
  • Longest observed run was ~7.45 minutes
  • No failures or timeout issues were found
  • All runs concluded successfully with a significant margin from the new 15-minute limit
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for any TODO/FIXME comments related to timing issues
rg -g "*.yml" "TODO|FIXME|NOTE" .github/workflows/

Length of output: 52


Script:

#!/bin/bash
# Check workflow run history to analyze job durations
gh run list --workflow=update-dependencies-5.yml --limit 50 --json durationMs,conclusion | jq -r '.[] | select(.conclusion=="success") | .durationMs' | awk '{printf "%.2f minutes\n", $1/60000}'

# Also check if there are any failed runs that might indicate timeout issues
gh run list --workflow=update-dependencies-5.yml --limit 50 --json conclusion,durationMs | jq -r '.[] | select(.conclusion=="failure" or .conclusion=="timed_out") | {conclusion: .conclusion, duration: (.durationMs/60000|tostring+" minutes")}'

Length of output: 912


Script:

#!/bin/bash
# Check workflow run history using startedAt and updatedAt to calculate durations
gh run list --workflow=update-dependencies-5.yml --limit 50 --json conclusion,startedAt,updatedAt | \
jq -r '.[] | select(.conclusion != null) | {
  conclusion: .conclusion,
  duration: (((.updatedAt | fromdateiso8601) - (.startedAt | fromdateiso8601))/60 | tostring + " minutes")
}'

Length of output: 3710

.github/workflows/update-dependencies-2.yml (1)

Line range hint 23-28: Verify timeout sufficiency for multiple solutions

This workflow processes multiple solutions (ShiftRegister.sln, Card.sln) within the same 15-minute timeout as workflows handling single solutions. We should verify this is sufficient.

Consider:

  1. Monitoring job duration specifically for workflows handling multiple solutions
  2. Documenting the rationale for solution grouping in the workflow comments
✅ Verification successful

Let me gather more information about the actual solutions to better assess the complexity.


Let me try one more verification with a corrected query for workflow duration:


15-minute timeout is sufficient for these solutions

Based on the evidence:

  • Recent runs complete successfully in 3-6 minutes, well within the timeout
  • Card.sln contains 4 projects (Card, Mifare, Ndef, Ultralight)
  • ShiftRegister.sln contains 2 projects (ShiftRegister, ShiftRegister-driver)
  • Both solutions have reasonable complexity and the workflow consistently completes in under half of the allocated time
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Count solutions per workflow to compare complexity
echo "Solutions per workflow:"
for i in {1..5}; do
  echo -n "Round $i: "
  grep -A 5 "solutionsToCheck:" .github/workflows/update-dependencies-$i.yml | grep ".sln" | wc -l
done

Length of output: 781


Script:

#!/bin/bash
# Check the size and complexity of solutions
echo "=== Round 1 solutions ==="
grep -A 5 "solutionsToCheck:" .github/workflows/update-dependencies-1.yml | grep ".sln"

echo -e "\n=== Round 2 solutions ==="
grep -A 5 "solutionsToCheck:" .github/workflows/update-dependencies-2.yml | grep ".sln"

# Check historical runs if available
echo -e "\n=== Recent workflow runs ==="
gh run list --workflow=update-dependencies-2.yml --limit 5 --json conclusion,durationInMinutes,createdAt \
  --jq '.[] | "\(.conclusion): \(.durationInMinutes)min [\(.createdAt)]"'

Length of output: 1011


Script:

#!/bin/bash
# Check recent workflow runs with correct fields
echo "=== Recent workflow runs ==="
gh run list --workflow=update-dependencies-2.yml --limit 5 --json conclusion,startedAt,updatedAt \
  --jq '.[] | {conclusion: .conclusion, duration: ((((.updatedAt | fromdateiso8601) - (.startedAt | fromdateiso8601))/60) | floor)} | "\(.conclusion): \(.duration)min"'

# Check solution complexity by looking at project references
echo -e "\n=== Solution complexity ==="
find . -name "ShiftRegister.sln" -o -name "Card.sln" | while read -r sln; do
  echo "=== $sln ==="
  grep -A 5 "Project(" "$sln" || echo "File not found"
done

Length of output: 1979

🧰 Tools
🪛 yamllint (1.35.1)

[error] 25-25: trailing spaces

(trailing-spaces)

.github/workflows/update-dependencies-1.yml (1)

10-11: LGTM! Schedule and timeout changes look reasonable.

The changes align well with the PR objectives:

  • Moving to Mon,Wed provides better spacing for team support
  • 02:00 UTC timing is more suitable
  • 15-minute timeout with sequential rounds is more efficient than the previous 180-minute timeout

Let's verify the schedule coordination across all workflow files:

Also applies to: 22-22

✅ Verification successful

Schedule coordination verified across all update-dependencies workflows

The verification confirms proper sequential scheduling:

  • update-dependencies-1: 02:00 UTC Mon,Wed
  • update-dependencies-2: 02:15 UTC Mon,Wed
  • update-dependencies-3: 02:30 UTC Mon,Wed
  • update-dependencies-4: 02:45 UTC Mon,Wed
  • update-dependencies-5: 05:45 UTC Mon,Wed

All workflows are properly coordinated with 15-minute intervals, except for update-dependencies-5 which runs at a later time (likely intentional for a different dependency group).

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify consistent scheduling pattern across all update-dependencies workflows
# Expected: Sequential 15-minute intervals starting at 02:00 UTC on Mon,Wed

echo "Checking scheduling patterns in all update-dependencies workflows..."
rg -A 2 "cron:" .github/workflows/update-dependencies-*.yml

Length of output: 1489

🧰 Tools
🪛 yamllint (1.35.1)

[warning] 11-11: too many spaces after colon

(colons)

.github/workflows/update-dependencies-4.yml (2)

10-12: LGTM! Clear and well-documented schedule change.

The added comments effectively explain the timing rationale, making the configuration more maintainable.


Line range hint 24-24: Verify if 180-minute timeout is still needed

While other workflow files have been updated to use 15-minute timeouts, this file retains the 180-minute timeout. Although there's a comment explaining it's due to "lots of solutions to update", this seems inconsistent with the PR's changes.

Consider:

  1. If this workflow truly needs more time, the comment should be expanded to explain the specific reason
  2. If not, align with the 15-minute timeout pattern used in other workflows

Let's check the number of solutions this workflow handles compared to others:

✅ Verification successful

180-minute timeout is justified for workflow-4 due to wildcard pattern matching all solutions

The longer timeout in update-dependencies-4.yml is warranted because:

  • It uses a wildcard pattern *.sln that potentially processes all 145 solution files in the repository
  • Other workflows handle specific solutions (1-8 solutions each) and use 15-minute timeouts
  • The existing comment "Non default as IoT devices has lots of solutions to update!" accurately reflects this scope difference

However, the comment could be more specific:

      timeout-minutes: 180 # Processing all *.sln files (potentially 145+ solutions) for IoT devices
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Compare the number of solutions handled by each workflow
# to verify if the longer timeout is justified

echo "Analyzing solution counts in workflows..."
for f in .github/workflows/update-dependencies-*.yml; do
    echo "=== $f ==="
    # Count explicit solutions
    echo "Explicit solutions:"
    rg "solutionsToCheck:" -A 10 "$f" | grep -v "solutionsToCheck:" | grep "\.sln"
    # Check for wildcard patterns
    echo "Wildcard patterns:"
    rg "solutionsToCheck:.*\*\.sln" "$f"
done

Length of output: 2757


Script:

#!/bin/bash
# Let's check the timeout settings across all update-dependencies workflows
echo "=== Checking timeout settings ==="
for f in .github/workflows/update-dependencies-*.yml; do
    echo "File: $f"
    rg "timeout-minutes:" -A 1 "$f"
done

# Let's also check if there are any comments explaining the timeout
echo -e "\n=== Checking for timeout-related comments ==="
for f in .github/workflows/update-dependencies-*.yml; do
    echo "File: $f"
    rg -B 2 "timeout-minutes:" "$f"
done

# Let's count total .sln files in the repository to understand the scope
echo -e "\n=== Counting total .sln files ==="
fd -e sln | wc -l

Length of output: 3609

take into account PR merge and branch build.
Copy link

@coderabbitai coderabbitai bot left a 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

🧹 Nitpick comments (6)
.github/workflows/update-dependencies-3.yml (1)

10-12: Fix YAML formatting in cron schedule

The cron schedule line has extra spaces after the colon. While this works, it's better to follow YAML best practices.

-    - cron:  '00 03 * * Mon,Wed'
+    - cron: '00 03 * * Mon,Wed'
🧰 Tools
🪛 yamllint (1.35.1)

[warning] 12-12: too many spaces after colon

(colons)

.github/workflows/update-dependencies-5.yml (1)

10-12: Fix YAML formatting and verify timing coordination

  1. The cron schedule line has extra spaces after the colon.
  2. The 3-hour gap from round 4 seems appropriate for changes to propagate.
-    - cron:  '30 06 * * Mon,Wed'
+    - cron: '30 06 * * Mon,Wed'
🧰 Tools
🪛 yamllint (1.35.1)

[warning] 12-12: too many spaces after colon

(colons)

.github/workflows/update-dependencies-2.yml (2)

10-12: Fix YAML formatting in cron schedule

The cron schedule line has extra spaces after the colon.

-    - cron:  '30 02 * * Mon,Wed'
+    - cron: '30 02 * * Mon,Wed'
🧰 Tools
🪛 yamllint (1.35.1)

[warning] 12-12: too many spaces after colon

(colons)


Line range hint 10-28: Overall workflow coordination looks good

The changes across all workflow files show a well-thought-out coordination:

  1. Moved from Wed,Fri to Mon,Wed for better team support
  2. Sequential timing with appropriate gaps between rounds
  3. Clear comments explaining the timing rationale

However, I recommend:

  1. Document the rationale for the significant timeout reduction (180 → 15 minutes) in the PR description
  2. Consider a slightly longer timeout (perhaps 30 minutes) for workflows processing multiple solutions
🧰 Tools
🪛 yamllint (1.35.1)

[error] 25-25: trailing spaces

(trailing-spaces)

.github/workflows/update-dependencies-1.yml (2)

10-11: Schedule change looks good, fix formatting

The new schedule (Mon,Wed at 02:00 UTC) is a good choice as it:

  • Provides better alignment with work week for core team support
  • Allows buffer time for addressing any issues that arise

Fix the extra spaces in the cron expression:

-    - cron:  '00 02 * * Mon,Wed'
+    - cron: '00 02 * * Mon,Wed'
🧰 Tools
🪛 yamllint (1.35.1)

[warning] 11-11: too many spaces after colon

(colons)


Line range hint 1-35: Well-structured workflow design

The separation into multiple workflow files (1-5) with staggered schedules is a good architectural choice:

  • Manages dependencies in smaller, manageable batches
  • Reduces risk of timeouts
  • Provides better error isolation
  • Allows for parallel processing of updates

Consider documenting the workflow sequence and dependencies between rounds in the workflow files to help maintainers understand the overall process.

🧰 Tools
🪛 yamllint (1.35.1)

[warning] 11-11: too many spaces after colon

(colons)

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 6fdbaed and 3c00032.

📒 Files selected for processing (5)
  • .github/workflows/update-dependencies-1.yml (2 hunks)
  • .github/workflows/update-dependencies-2.yml (2 hunks)
  • .github/workflows/update-dependencies-3.yml (2 hunks)
  • .github/workflows/update-dependencies-4.yml (2 hunks)
  • .github/workflows/update-dependencies-5.yml (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • .github/workflows/update-dependencies-4.yml
🧰 Additional context used
🪛 yamllint (1.35.1)
.github/workflows/update-dependencies-1.yml

[warning] 11-11: too many spaces after colon

(colons)

.github/workflows/update-dependencies-2.yml

[warning] 12-12: too many spaces after colon

(colons)

.github/workflows/update-dependencies-3.yml

[warning] 12-12: too many spaces after colon

(colons)

.github/workflows/update-dependencies-5.yml

[warning] 12-12: too many spaces after colon

(colons)

🔇 Additional comments (3)
.github/workflows/update-dependencies-3.yml (1)

23-23: Verify if 15-minute timeout is sufficient

The timeout has been significantly reduced from 180 to 15 minutes. While reducing timeouts is good practice, please verify that 15 minutes is sufficient for:

  1. Checking and updating dependencies
  2. Creating and processing pull requests
  3. Handling any potential network delays or service issues
✅ Verification successful

15-minute timeout is sufficient based on historical data

Looking at the workflow execution history over the past few months, all runs have completed successfully with execution times consistently between 3-7 minutes. The longest run took about 6 minutes and 53 seconds (Nov 13), which is well within the 15-minute timeout limit. The workflow appears to be stable and efficient, focusing on updating dependencies for a single solution file (CharacterLcd.sln).

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check historical workflow run times to verify if 15 minutes is sufficient

gh api -X GET "/repos/nanoframework/nanoFramework.IoT.Device/actions/workflows/update-dependencies-3.yml/runs" \
  --jq '.workflow_runs[] | select(.conclusion != null) | {conclusion: .conclusion, duration: (.updated_at | fromdateiso8601 - (.created_at | fromdateiso8601))/60}'

Length of output: 660


Script:

#!/bin/bash
# Let's try a simpler approach to check workflow runs
gh api -X GET "/repos/nanoframework/nanoFramework.IoT.Device/actions/workflows/update-dependencies-3.yml/runs" \
  --jq '.workflow_runs[] | select(.conclusion != null) | {conclusion: .conclusion, created_at: .created_at, updated_at: .updated_at}'

# Also let's check the workflow file itself to understand the job better
cat .github/workflows/update-dependencies-3.yml

Length of output: 4111

.github/workflows/update-dependencies-2.yml (1)

Line range hint 23-28: Review timeout considering multiple solutions

The 15-minute timeout might be too aggressive considering this job processes multiple solutions (ShiftRegister.sln and Card.sln). Please verify that the timeout is sufficient for processing all solutions.

✅ Verification successful

15-minute timeout is sufficient based on historical runs

Looking at the workflow run history, all executions have been consistently completing successfully within 3-5 minutes (comparing created_at and updated_at timestamps). The current 15-minute timeout provides ample buffer for processing both solutions, even accounting for potential variations in execution time.

Recent examples:

  • Dec 16: ~4.5 minutes
  • Dec 13: ~6 minutes
  • Dec 11: ~4 minutes
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check historical workflow run times for jobs with multiple solutions

gh api -X GET "/repos/nanoframework/nanoFramework.IoT.Device/actions/workflows/update-dependencies-2.yml/runs" \
  --jq '.workflow_runs[] | select(.conclusion != null) | {conclusion: .conclusion, duration: (.updated_at | fromdateiso8601 - (.created_at | fromdateiso8601))/60, created_at: .created_at}'

Length of output: 710


Script:

#!/bin/bash
# Check historical workflow run times for jobs with multiple solutions
# Using simpler jq expression without date calculations

gh api -X GET "/repos/nanoframework/nanoFramework.IoT.Device/actions/workflows/update-dependencies-2.yml/runs" \
  --jq '.workflow_runs[] | select(.conclusion != null) | {conclusion: .conclusion, created_at: .created_at, updated_at: .updated_at}'

# Also check the workflow file content to understand the job structure
cat .github/workflows/update-dependencies-2.yml

Length of output: 4129

🧰 Tools
🪛 yamllint (1.35.1)

[error] 25-25: trailing spaces

(trailing-spaces)

.github/workflows/update-dependencies-1.yml (1)

22-22: Verify if 15-minute timeout is sufficient

The timeout has been significantly reduced from 180 to 15 minutes. While the comment suggests this is intentional as part of a staged approach, we should verify this is sufficient.

✅ Verification successful

15-minute timeout aligns with other dependency update workflows

The timeout configuration is consistent with the repository's pattern:

  • All dependency update workflows (1-5) use the same 15-minute timeout, except for update-dependencies-3.yml which has 120 minutes due to handling multiple solutions
  • The comments indicate this is an intentional staged approach where workflows run sequentially
  • The comment "The next round will start after" confirms this is part of a pipeline design where workflows are chained
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check historical job durations to verify if 15 minutes is sufficient

# Get workflow run history focusing on the job duration
gh api -X GET repos/nanoframework/nanoFramework.IoT.Device/actions/workflows/update-dependencies-1.yml/runs \
  --jq '.workflow_runs[] | select(.conclusion != null) | {conclusion: .conclusion, duration: (.updated_at | fromdateiso8601 - (.created_at | fromdateiso8601))}' \
  | head -n 10

Length of output: 341


Script:

#!/bin/bash
# Let's try a different approach to check workflow runs
# First, let's verify the workflow file and its history
git log -p -- .github/workflows/update-dependencies-1.yml | grep -A 5 "timeout-minutes:"

# Also check if there are similar timeout configurations in other workflow files
fd "yml$" .github/workflows/ --exec grep -l "timeout-minutes"

# And check their timeout values
fd "yml$" .github/workflows/ --exec grep -B 2 -A 2 "timeout-minutes" {}

Length of output: 1357

@josesimoes josesimoes changed the title [gh-workflow] Improve update-dependencies timeschedule Work CI-CD Dec 17, 2024
Copy link
Member

@josesimoes josesimoes left a comment

Choose a reason for hiding this comment

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

LGTM.
Care to follow coderabbitai suggestions and fix the yaml formatting on the cron expressions? 😉

@networkfusion
Copy link
Member Author

@CodeRabbit full review

Copy link

coderabbitai bot commented Dec 18, 2024

✅ Actions performed

Full review triggered.

@networkfusion
Copy link
Member Author

LGTM. Care to follow coderabbitai suggestions and fix the yaml formatting on the cron expressions? 😉

I had already done them but code rabbit doesnt pick them up until you do another full review (initiated).

@networkfusion
Copy link
Member Author

@CodeRabbit full review

Copy link

coderabbitai bot commented Dec 18, 2024

✅ Actions performed

Full review triggered.

Copy link

@coderabbitai coderabbitai bot left a 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

🧹 Nitpick comments (3)
.github/workflows/update-dependencies-5.yml (1)

23-23: Document timeout rationale

While the comment states "It should not take longer!", it would be helpful to document why 15 minutes is considered sufficient, especially for the final round which might need to handle any accumulated delays or retries from previous rounds.

Consider adding more detailed documentation about the timeout rationale:

-      timeout-minutes: 15 # It should not take longer!
+      timeout-minutes: 15 # Based on historical runs, updates typically complete within 10 minutes
.github/workflows/update-dependencies-2.yml (2)

23-23: Improve timeout documentation

The comment about the next round is helpful but could be clearer about timing expectations.

Consider updating the comment to be more specific:

-      timeout-minutes: 15 # The next round will start after (taking into account PR merge and build branch).
+      timeout-minutes: 15 # Round 3 starts at 03:00 UTC, allowing 30 minutes for completion and PR processing

Line range hint 1-1: Consider implementing workflow dependencies

The current design relies on time-based coordination between workflow rounds. While the spacing is well-thought-out, consider using GitHub Actions' workflow_run event to create explicit dependencies between rounds. This would provide more reliable sequencing and potentially allow for shorter time gaps between rounds.

Example implementation:

on:
  workflow_run:
    workflows: ["Update dependencies (round N-1)"]
    types:
      - completed

This would ensure each round starts immediately after the previous round succeeds, rather than waiting for a fixed time interval.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between f8b8fb2 and a1b527c.

📒 Files selected for processing (5)
  • .github/workflows/update-dependencies-1.yml (2 hunks)
  • .github/workflows/update-dependencies-2.yml (2 hunks)
  • .github/workflows/update-dependencies-3.yml (2 hunks)
  • .github/workflows/update-dependencies-4.yml (2 hunks)
  • .github/workflows/update-dependencies-5.yml (2 hunks)
🔇 Additional comments (8)
.github/workflows/update-dependencies-3.yml (2)

10-12: Verify the timing sequence between rounds

The comment states this runs 30 minutes after round 2, but looking at the cron expression ('00 03 * * Mon,Wed') and round 2's schedule ('30 02 * * Mon,Wed'), this actually runs 30 minutes after the expected completion of round 2, which provides a good buffer for changes to propagate.


23-23: Consider increasing the timeout value

The timeout has been significantly reduced from 180 to 15 minutes. While optimizing timeouts is good, 15 minutes might be too aggressive for dependency updates that could involve multiple packages or network delays.

Consider setting a more conservative timeout of 30-45 minutes initially and adjust based on actual run durations.

.github/workflows/update-dependencies-5.yml (1)

10-12: LGTM: Well-spaced final round schedule

The 06:30 UTC schedule for the final round provides adequate buffer time (3 hours) from round 4, which helps ensure all previous rounds complete successfully.

.github/workflows/update-dependencies-2.yml (1)

10-12: LGTM: Well-coordinated schedule with round 1

The 30-minute spacing from round 1 is well-documented and provides a good balance between parallel execution and allowing changes to propagate.

.github/workflows/update-dependencies-1.yml (2)

22-22: LGTM! Timeout adjustment is appropriate.

The 15-minute timeout is reasonable for the specified solutions and maintains efficient workflow coordination.


10-12: LGTM! Schedule change aligns with objectives.

The shift to Monday/Wednesday at 02:00 UTC is well-positioned as the first round of updates, allowing core team support during their active hours.

Let's verify the coordination with other workflow schedules:

✅ Verification successful

Schedule coordination is well-maintained across all update-dependency workflows

The verification confirms proper sequential scheduling:

  • Round 1 starts at 02:00 UTC
  • Rounds 2-4 follow with 30-minute intervals (02:30, 03:00, 03:30 UTC)
  • Round 5 runs at 06:30 UTC with a longer gap for propagation
  • All rounds consistently run on Mon,Wed
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify scheduling coordination across all update-dependencies workflows
# Expect: Sequential scheduling with appropriate gaps between rounds

echo "Checking all update-dependencies workflow schedules..."
for f in .github/workflows/update-dependencies-*.yml; do
  echo "=== $f ==="
  grep -A 1 "# Round" "$f"
  grep "cron:" "$f"
done

Length of output: 2468

.github/workflows/update-dependencies-4.yml (2)

10-12: LGTM! Well-coordinated schedule adjustment.

The timing at 03:30 UTC provides appropriate spacing after round 3, with clear documentation of the coordination logic.


Line range hint 23-27: LGTM! Timeout adjustment with proper scope management.

The 120-minute timeout is well-justified for the broader scope of IoT device solutions. The extensive exclusion list helps manage the workload effectively.

Let's monitor the actual execution times to validate this timeout:

✅ Verification successful

✅ Timeout of 120 minutes is appropriate and validated by actual run times

Recent workflow executions consistently complete within 75-85 minutes, showing that the 120-minute timeout provides adequate buffer while being more resource-efficient than the previous 180-minute limit. All runs completed successfully, indicating the timeout is not causing any issues.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check recent workflow run durations
# Expect: Execution times within the 120-minute limit

gh run list --workflow=update-dependencies-4.yml --limit 5 --json durationInMinutes \
  | jq -r '.[] | .durationInMinutes'

Length of output: 363


Script:

#!/bin/bash
# Let's try with startedAt and updatedAt to calculate duration
gh run list --workflow=update-dependencies-4.yml --limit 5 --json startedAt,updatedAt,conclusion \
  | jq -r '.[] | "\(.startedAt) -> \(.updatedAt) [\(.conclusion)]"'

Length of output: 439

@networkfusion networkfusion merged commit 5e60df6 into nanoframework:develop Dec 18, 2024
3 checks passed
@networkfusion networkfusion deleted the improve-gh-workflow-1 branch December 18, 2024 14:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants