Conversation
iii-i
left a comment
There was a problem hiding this comment.
LGTM. I will bug the gaplib maintainers about the .NET 8 support.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #1782 +/- ##
===========================================
+ Coverage 82.10% 82.45% +0.34%
===========================================
Files 136 136
Lines 10969 11045 +76
Branches 2716 2822 +106
===========================================
+ Hits 9006 9107 +101
+ Misses 1266 1240 -26
- Partials 697 698 +1 ☔ View full report in Codecov by Sentry. |
741f111 to
0946184
Compare
|
Yes. It's unfortunate that it's now broken for that long; the gaplib maintainer told me that the latest runner version was updated to use dotnet6 instead of dotnet8, and this made the fix complicated, so he wanted to wait until they bump it back to dotnet8, which happened today: actions/runner#3500. So hopefully there will be some progress now. |
|
With the latest set of changes, I'm finally able to build gaplib again (specifically, this commit: https://github.com/anup-kodlekere/gaplib/tree/f5085ab2e51fad20329b441f3c5475c50e4ea1bd). Could you please give it a try? |
0946184 to
7a11c77
Compare
|
Warning Rate limit exceeded@Dead2 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 3 minutes and 46 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 selected for processing (8)
WalkthroughThe pull request introduces comprehensive updates to the self-hosted GitHub Actions runner configuration for the IBM Z (s390x) architecture. The changes focus on upgrading the .NET SDK from version 6.0 to 8.0, modifying the runner setup scripts, and improving the service management process. The modifications include updating the Dockerfile, creating new scripts for runner registration and rebuilding, adjusting the systemd service configuration, and removing previously used patch and configuration files. Changes
Sequence DiagramsequenceDiagram
participant Script as Actions Runner Script
participant GitHub as GitHub API
participant Runner as GitHub Runner
Script->>Script: Check registration token
alt Token expired or missing
Script->>GitHub: Request new registration token
GitHub-->>Script: Return registration token
Script->>Script: Save token to file
end
Script->>Runner: Configure runner
Script->>Runner: Start runner
Runner->>GitHub: Register for repository
Runner->>GitHub: Ready to execute jobs
The sequence diagram illustrates the high-level process of the new Actions runner script, showing how it manages registration tokens and prepares the runner for job execution. 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 (
|
7a11c77 to
5d5a406
Compare
5d5a406 to
aff1d6a
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (4)
arch/s390/self-hosted-builder/actions-runner (1)
40-49: Consider additional runner configuration optionsThe runner configuration could benefit from additional options for better control and security.
Consider adding these options to config.sh:
./config.sh \ --url "https://github.com/$REPO" \ --token "$REG_TOKEN" \ --unattended \ --disableupdate \ --replace \ --labels z15 \ - --ephemeral + --ephemeral \ + --name "z15-$(hostname)" \ + --work "_work" \ + --runasservicearch/s390/self-hosted-builder/actions-runner.Dockerfile (1)
41-42: Verify script permissionsThe scripts are correctly placed in
/usr/bin/, but ensure they have proper executable permissions.Add explicit permissions:
-COPY entrypoint /usr/bin/ -COPY actions-runner /usr/bin/ +COPY entrypoint /usr/bin/ +COPY actions-runner /usr/bin/ +RUN chmod +x /usr/bin/entrypoint /usr/bin/actions-runnerarch/s390/README.md (2)
229-242: Documentation improvements look good, with minor formatting suggestionsThe configuration instructions are clear and comprehensive. Consider these markdown improvements:
- Remove trailing punctuation from headings
- Add language specifier to code blocks
- Use markdown link syntax instead of bare URL
-### Create a config file, needs github personal access token. +### Create a config file + +Requires a GitHub personal access token with the following permissions: +- Repo Admin (read/write) +- Org Self-hosted runners (read/write) -For details, consult -https://docs.github.com/en/rest/actions/self-hosted-runners?apiVersion=2022-11-28#create-a-registration-token-for-a-repository +For details, see [GitHub documentation](https://docs.github.com/en/rest/actions/self-hosted-runners?apiVersion=2022-11-28#create-a-registration-token-for-a-repository) -``` +```bash REPO=<owner>/<name> PAT_TOKEN=<github_pat_***><details> <summary>🧰 Tools</summary> <details> <summary>🪛 Markdownlint (0.37.0)</summary> 229-229: Punctuation: '.' Trailing punctuation in heading (MD026, no-trailing-punctuation) --- 234-234: Punctuation: ':' Trailing punctuation in heading (MD026, no-trailing-punctuation) --- 240-240: Punctuation: ':' Trailing punctuation in heading (MD026, no-trailing-punctuation) --- 232-232: null Bare URL used (MD034, no-bare-urls) --- 235-235: null Fenced code blocks should have a language specified (MD040, fenced-code-language) --- 241-241: null Fenced code blocks should have a language specified (MD040, fenced-code-language) --- 235-235: Expected: indented; Actual: fenced Code block style (MD046, code-block-style) --- 241-241: Expected: indented; Actual: fenced Code block style (MD046, code-block-style) </details> </details> --- `245-249`: **Add service verification step** The service setup instructions are correct. Consider adding a verification step: ```diff sudo cp self-hosted-builder/actions-runner.service /etc/systemd/system/ sudo systemctl daemon-reload +sudo systemctl status actions-runner # Verify service configuration🧰 Tools
🪛 Markdownlint (0.37.0)
245-245: Punctuation: '.'
Trailing punctuation in heading(MD026, no-trailing-punctuation)
246-246: null
Fenced code blocks should have a language specified(MD040, fenced-code-language)
246-246: Expected: indented; Actual: fenced
Code block style(MD046, code-block-style)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
arch/s390/README.md(1 hunks)arch/s390/self-hosted-builder/actions-runner(1 hunks)arch/s390/self-hosted-builder/actions-runner-rebuild.sh(1 hunks)arch/s390/self-hosted-builder/actions-runner.Dockerfile(2 hunks)arch/s390/self-hosted-builder/actions-runner.service(1 hunks)arch/s390/self-hosted-builder/fs/usr/bin/actions-runner(0 hunks)arch/s390/self-hosted-builder/runner-global.json(0 hunks)arch/s390/self-hosted-builder/runner-s390x.patch(0 hunks)
💤 Files with no reviewable changes (3)
- arch/s390/self-hosted-builder/runner-global.json
- arch/s390/self-hosted-builder/fs/usr/bin/actions-runner
- arch/s390/self-hosted-builder/runner-s390x.patch
🧰 Additional context used
🪛 Markdownlint (0.37.0)
arch/s390/README.md
253-253: null
Dollar signs used before commands without showing output
(MD014, commands-show-output)
229-229: Punctuation: '.'
Trailing punctuation in heading
(MD026, no-trailing-punctuation)
234-234: Punctuation: ':'
Trailing punctuation in heading
(MD026, no-trailing-punctuation)
240-240: Punctuation: ':'
Trailing punctuation in heading
(MD026, no-trailing-punctuation)
245-245: Punctuation: '.'
Trailing punctuation in heading
(MD026, no-trailing-punctuation)
251-251: Punctuation: '.'
Trailing punctuation in heading
(MD026, no-trailing-punctuation)
232-232: null
Bare URL used
(MD034, no-bare-urls)
235-235: null
Fenced code blocks should have a language specified
(MD040, fenced-code-language)
241-241: null
Fenced code blocks should have a language specified
(MD040, fenced-code-language)
246-246: null
Fenced code blocks should have a language specified
(MD040, fenced-code-language)
252-252: null
Fenced code blocks should have a language specified
(MD040, fenced-code-language)
257-257: null
Fenced code blocks should have a language specified
(MD040, fenced-code-language)
263-263: null
Fenced code blocks should have a language specified
(MD040, fenced-code-language)
235-235: Expected: indented; Actual: fenced
Code block style
(MD046, code-block-style)
241-241: Expected: indented; Actual: fenced
Code block style
(MD046, code-block-style)
246-246: Expected: indented; Actual: fenced
Code block style
(MD046, code-block-style)
252-252: Expected: indented; Actual: fenced
Code block style
(MD046, code-block-style)
257-257: Expected: indented; Actual: fenced
Code block style
(MD046, code-block-style)
263-263: Expected: indented; Actual: fenced
Code block style
(MD046, code-block-style)
🔇 Additional comments (5)
arch/s390/self-hosted-builder/actions-runner.service (1)
13-14: LGTM: Improved container shutdown timeouts
The increased timeouts (30s for stop, 10s for stop-post) provide better graceful shutdown handling for the container. The total stop time (40s) is well within the service's TimeoutStopSec (61s).
Let's verify the systemd configuration:
✅ Verification successful
Let me gather more information about the service file to verify the complete configuration:
Systemd service configuration is correctly structured
The service configuration is well-designed with appropriate timeout values:
- TimeoutStopSec=61s provides sufficient overhead for the combined stop operations (30s + 10s = 40s)
- Type=forking is correctly set for podman container service
- Stop timeouts are properly ordered: primary stop (30s) followed by shorter post-stop (10s)
- No other podman services in the repository to conflict with these settings
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify systemd service configuration
systemd-analyze verify arch/s390/self-hosted-builder/actions-runner.service
# Check if similar services in the repo use different timeout values
rg "ExecStop.*podman stop -t" --type systemd
Length of output: 456
Script:
#!/bin/bash
# Check the complete service file content
cat arch/s390/self-hosted-builder/actions-runner.service
# Look for other podman-related systemd services for comparison
fd -e service | xargs grep -l "podman" || true
Length of output: 641
arch/s390/self-hosted-builder/actions-runner.Dockerfile (3)
6-10: LGTM: Package installation looks good
The addition of sed package is appropriate as it's required for the subsequent version string modification.
11-12: LGTM: .NET SDK 8.0 installation verified
The upgrade to .NET SDK 8.0 aligns with the PR objectives, and the version verification step is a good practice.
18-20: Consider security implications of remote patch download
While the patch download from a specific GitHub repository works, consider:
- Pinning to a specific commit hash instead of using the main branch
- Verifying the patch file integrity using checksums
✅ Verification successful
Pin patch download to specific commit hash for security
The patch file was last modified on November 11, 2024, as part of significant changes to the repository. For better security and reproducibility:
wget https://github.com/anup-kodlekere/gaplib/raw/f5085ab2e51fad20329b441f3c5475c50e4ea1bd/build-files/runner-sdk-8.patch && \🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if the patch file has changed recently
gh api \
-H "Accept: application/vnd.github+json" \
repos/anup-kodlekere/gaplib/commits \
--jq '.[0] | {sha: .sha, date: .commit.author.date, message: .commit.message}'
Length of output: 1848
arch/s390/README.md (1)
256-264: LGTM: Automated rebuild setup is well-documented
The addition of automated rebuilds via cron job is a good improvement for maintenance.
🧰 Tools
🪛 Markdownlint (0.37.0)
257-257: null
Fenced code blocks should have a language specified
(MD040, fenced-code-language)
263-263: null
Fenced code blocks should have a language specified
(MD040, fenced-code-language)
257-257: Expected: indented; Actual: fenced
Code block style
(MD046, code-block-style)
263-263: Expected: indented; Actual: fenced
Code block style
(MD046, code-block-style)
aff1d6a to
63b4b9d
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (7)
arch/s390/self-hosted-builder/actions-runner.Dockerfile (1)
41-42: Check script permissions.
When scripts are copied into /usr/bin, ensure they are executable to avoid runtime errors.You can add a RUN chmod +x /usr/bin/entrypoint /usr/bin/actions-runner line after the COPY commands.
arch/s390/self-hosted-builder/actions-runner-rebuild.sh (3)
4-7: Validate config before proceeding.
You are checking /etc/actions-runner’s existence. Consider adding extra validation (length check on token) to prevent build issues if the file is empty or has invalid content.
10-19: Handle external Dockerfile fetch gracefully.
If the remote file doesn’t exist or cannot be accessed, ensure the script fails with a clear message to the user.
34-34: Consider specifying container resource constraints.
Using flags like --memory or --cpus ensures a more controlled environment, especially on production servers.arch/s390/README.md (3)
229-230: Fix trailing punctuation in heading.
“### Create a config file, needs github personal access token.” triggers a markdownlint warning (MD026).🧰 Tools
🪛 Markdownlint (0.37.0)
229-229: Punctuation: '.'
Trailing punctuation in heading(MD026, no-trailing-punctuation)
232-232: Use bracketed link syntax for references.
The bare URL at line 232 triggers MD034. This improves readability and compliance with markdown style guides.🧰 Tools
🪛 Markdownlint (0.37.0)
232-232: null
Bare URL used(MD034, no-bare-urls)
235-235: Specify language for fenced code blocks.
Specifying a language (e.g., bash, yaml) helps with syntax highlighting.Also applies to: 241-241, 246-246, 252-252, 257-257, 263-263
🧰 Tools
🪛 Markdownlint (0.37.0)
235-235: null
Fenced code blocks should have a language specified(MD040, fenced-code-language)
235-235: Expected: indented; Actual: fenced
Code block style(MD046, code-block-style)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
arch/s390/README.md(1 hunks)arch/s390/self-hosted-builder/actions-runner(1 hunks)arch/s390/self-hosted-builder/actions-runner-rebuild.sh(1 hunks)arch/s390/self-hosted-builder/actions-runner.Dockerfile(2 hunks)arch/s390/self-hosted-builder/actions-runner.service(1 hunks)arch/s390/self-hosted-builder/fs/usr/bin/actions-runner(0 hunks)arch/s390/self-hosted-builder/runner-global.json(0 hunks)arch/s390/self-hosted-builder/runner-s390x.patch(0 hunks)
💤 Files with no reviewable changes (3)
- arch/s390/self-hosted-builder/runner-global.json
- arch/s390/self-hosted-builder/fs/usr/bin/actions-runner
- arch/s390/self-hosted-builder/runner-s390x.patch
🚧 Files skipped from review as they are similar to previous changes (2)
- arch/s390/self-hosted-builder/actions-runner
- arch/s390/self-hosted-builder/actions-runner.service
🧰 Additional context used
🪛 Markdownlint (0.37.0)
arch/s390/README.md
253-253: null
Dollar signs used before commands without showing output
(MD014, commands-show-output)
229-229: Punctuation: '.'
Trailing punctuation in heading
(MD026, no-trailing-punctuation)
234-234: Punctuation: ':'
Trailing punctuation in heading
(MD026, no-trailing-punctuation)
240-240: Punctuation: ':'
Trailing punctuation in heading
(MD026, no-trailing-punctuation)
245-245: Punctuation: '.'
Trailing punctuation in heading
(MD026, no-trailing-punctuation)
251-251: Punctuation: '.'
Trailing punctuation in heading
(MD026, no-trailing-punctuation)
232-232: null
Bare URL used
(MD034, no-bare-urls)
235-235: null
Fenced code blocks should have a language specified
(MD040, fenced-code-language)
241-241: null
Fenced code blocks should have a language specified
(MD040, fenced-code-language)
246-246: null
Fenced code blocks should have a language specified
(MD040, fenced-code-language)
252-252: null
Fenced code blocks should have a language specified
(MD040, fenced-code-language)
257-257: null
Fenced code blocks should have a language specified
(MD040, fenced-code-language)
263-263: null
Fenced code blocks should have a language specified
(MD040, fenced-code-language)
235-235: Expected: indented; Actual: fenced
Code block style
(MD046, code-block-style)
241-241: Expected: indented; Actual: fenced
Code block style
(MD046, code-block-style)
246-246: Expected: indented; Actual: fenced
Code block style
(MD046, code-block-style)
252-252: Expected: indented; Actual: fenced
Code block style
(MD046, code-block-style)
257-257: Expected: indented; Actual: fenced
Code block style
(MD046, code-block-style)
263-263: Expected: indented; Actual: fenced
Code block style
(MD046, code-block-style)
🔇 Additional comments (6)
arch/s390/self-hosted-builder/actions-runner.Dockerfile (3)
18-20: Ensure the patch is up to date.
You are applying a remote patch to the runner. If the patch content changes, a mismatch could break the build.
Consider verifying the contents of runner-sdk-8.patch before applying:
✅ Verification successful
Let me try a different approach to verify the patch file's existence and contents.
The patch file is accessible and contains expected changes for s390x and ppc64le architecture support
The patch file from the remote repository is successfully retrieved and contains legitimate changes to add support for s390x and ppc64le architectures in the GitHub Actions runner. The changes include:
- Adding architecture definitions for s390x and ppc64le
- Modifying build configurations for these architectures
- Adding node download support for these platforms
- Adjusting dependency checks for these architectures
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Download patch to a temporary file and do a diff to confirm no unexpected changes
wget https://github.com/anup-kodlekere/gaplib/raw/refs/heads/main/build-files/runner-sdk-8.patch -O /tmp/runner-sdk-8.patch
diff /tmp/runner-sdk-8.patch arch/s390/self-hosted-builder/runner-sdk-8.patch || echo "Patch differences found."
Length of output: 498
Script:
#!/bin/bash
# Check if the patch file exists in the repository
fd runner-sdk-8.patch
# Use curl instead of wget to download the patch
curl -L https://github.com/anup-kodlekere/gaplib/raw/refs/heads/main/build-files/runner-sdk-8.patch 2>/dev/null || echo "Failed to download patch"
Length of output: 15861
11-11: Validate the .NET SDK version.
The .NET SDK 8.0 is relatively new. Confirm that your application relies on or is compatible with this version.
Use this script to see if references to legacy .NET versions are present in the codebase:
✅ Verification successful
The .NET SDK 8.0 installation appears to be intentional and appropriate
The codebase shows no references to older .NET SDK versions, and the Dockerfile explicitly checks the installed SDK version after installation. The only .NET-related content found is in the s390 architecture's self-hosted builder, which is specifically designed with .NET SDK 8.0. The README.md in the same directory acknowledges potential future updates to the actions-runner, showing awareness of the .NET SDK versioning.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Searching for references to older .NET versions
rg "(dotnet-sdk-3\.1|dotnet-sdk-5\.0|dotnet-sdk-6\.0|dotnet-sdk-7\.0)"
Length of output: 72
Script:
#!/bin/bash
# Let's search for any .NET SDK or runtime related files and configurations
rg -i "dotnet|\.net" -g "!*.{dll,exe,pdb}"
# Also search for project files that might indicate .NET version requirements
fd -e csproj -e sln -x cat {}
Length of output: 1973
6-6: Consider pinning package versions.
Installing packages without specifying versions can lead to unplanned breakage or inconsistent builds when upstream packages change.
You can run the following script to locate potential version pinning best practices in your project’s Dockerfiles:
arch/s390/self-hosted-builder/actions-runner-rebuild.sh (3)
1-2: Good practice to enable error handling.
Using “set -ex” helps to exit on errors and echo commands. This is an excellent way to identify failing steps quickly.
30-31: Validate successful image build.
If the build fails (e.g., missing Dockerfile or SDK mismatch), consider exiting the script to avoid proceeding with a broken image.
40-42: Clean up logic check.
You check stdin with if [ -t 0 ] but commented out code suggests you might want to check -t 1 as well. Double-check that logic is correct for your scenario.
63b4b9d to
a178cb3
Compare
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores