fix: dotnet SDK#469
Merged
Merged
Conversation
📝 WalkthroughWalkthroughThis PR addresses C# code generation failures by introducing a custom JSON serialization template, updating the generator configuration to use it, and refreshing the build environment for multi-architecture support. The core fix adds a ChangesC# JSON Serialization Template and Code Generation
Build Environment Multi-architecture Support
🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 1 | ❌ 4❌ Failed checks (3 warnings, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
scripts/generate.sh (1)
11-15:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winGuard the config glob to avoid hard-failing on zero matches.
With Bash default globbing,
config/client/*.ymlcan remain literal when no files match, andenvsubstthen fails on a non-existent path.Suggested hardening
+shopt -s nullglob for f in config/client/*.yml do echo "Processing $f..." envsubst < "${f}" > "${f}.proc.yml" done +shopt -u nullglob🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/generate.sh` around lines 11 - 15, The glob "config/client/*.yml" can be left literal when no files match causing envsubst to fail; update the script around the for loop (the for f in config/client/*.yml ... do ... done block and the use of envsubst and variable f) to guard the glob—either enable nullglob temporarily (shopt -s nullglob; ...; shopt -u nullglob) before iterating, or test for matches first (e.g. use compgen -G "config/client/*.yml" >/dev/null || exit 0) or skip when [ ! -e "$f" ] inside the loop—so envsubst only runs for real files and the script does not hard-fail when there are zero matches.
🧹 Nitpick comments (2)
Dockerfile (2)
64-68: Go-swagger arch selection is correct; consider pinning the release for reproducibility/CI resilience.The
swagger_linux_${TARGETARCH}asset name matches go-swagger's publishedswagger_linux_amd64/swagger_linux_arm64assets, so the multi-arch selection is right. However,releases/latestagainst the unauthenticated GitHub API is rate-limited (60 req/hr/IP) on shared CI runners and silently floats the swagger version over time, making builds non-reproducible. Consider pinning a known version (and/or passing a token) so builds are deterministic and don't break on rate limits.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Dockerfile` around lines 64 - 68, Replace the dynamic GitHub "releases/latest" lookup used to set download_url (the curl | jq pipeline that builds swagger_linux_${TARGETARCH} and assigns download_url) with a deterministic approach: either hardcode a specific go-swagger release tag/version in the API URL (e.g. use /releases/tags/<VERSION> instead of /releases/latest) or add support for an authenticated request by reading a GITHUB_TOKEN env var and passing an Authorization: Bearer header to curl to avoid rate limits; update the curl/jq invocation that sets download_url and keep the subsequent curl -o /usr/local/bin/swagger and chmod steps unchanged so builds are reproducible and resilient to CI rate limits.
49-49: ⚡ Quick win
curl … | bashmasks download failures.Without
pipefail, the pipeline exits withbash's status, so a failed/partialcurl(network blip, 404) still produces a "successful" layer with no/partial dotnet install — surfacing only later as a confusingdotnet buildfailure. Same pattern applies to theory(line 10), node (line 29) and rust (line 60) installs.♻️ Enable pipefail for piped installs
+# Make piped installers fail the build if the download fails +SHELL ["/bin/bash", "-o", "pipefail", "-c"]Add this once near the top of the Dockerfile (after the base
FROM).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Dockerfile` at line 49, Add a SHELL directive immediately after the base FROM so /bin/bash runs with pipefail enabled, ensuring failures in piped RUN commands (the RUN curl ... | bash used for dotnet, the ory install on line 10, the node install on line 29, and the rust install on line 60) cause the build to fail fast; update the Dockerfile so subsequent RUN lines that pipe downloads into bash respect pipefail (or alternatively prefix each of those RUN commands with a shell invocation that enables pipefail) so partial/failed downloads are not treated as successful layers.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@scripts/generate.sh`:
- Around line 11-15: The glob "config/client/*.yml" can be left literal when no
files match causing envsubst to fail; update the script around the for loop (the
for f in config/client/*.yml ... do ... done block and the use of envsubst and
variable f) to guard the glob—either enable nullglob temporarily (shopt -s
nullglob; ...; shopt -u nullglob) before iterating, or test for matches first
(e.g. use compgen -G "config/client/*.yml" >/dev/null || exit 0) or skip when [
! -e "$f" ] inside the loop—so envsubst only runs for real files and the script
does not hard-fail when there are zero matches.
---
Nitpick comments:
In `@Dockerfile`:
- Around line 64-68: Replace the dynamic GitHub "releases/latest" lookup used to
set download_url (the curl | jq pipeline that builds swagger_linux_${TARGETARCH}
and assigns download_url) with a deterministic approach: either hardcode a
specific go-swagger release tag/version in the API URL (e.g. use
/releases/tags/<VERSION> instead of /releases/latest) or add support for an
authenticated request by reading a GITHUB_TOKEN env var and passing an
Authorization: Bearer header to curl to avoid rate limits; update the curl/jq
invocation that sets download_url and keep the subsequent curl -o
/usr/local/bin/swagger and chmod steps unchanged so builds are reproducible and
resilient to CI rate limits.
- Line 49: Add a SHELL directive immediately after the base FROM so /bin/bash
runs with pipefail enabled, ensuring failures in piped RUN commands (the RUN
curl ... | bash used for dotnet, the ory install on line 10, the node install on
line 29, and the rust install on line 60) cause the build to fail fast; update
the Dockerfile so subsequent RUN lines that pipe downloads into bash respect
pipefail (or alternatively prefix each of those RUN commands with a shell
invocation that enables pipefail) so partial/failed downloads are not treated as
successful layers.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 9adbceaa-43cf-4933-91a0-e05509d1c0a6
📒 Files selected for processing (3)
Dockerfileconfig/client/dotnet-templates/libraries/generichost/JsonConverter.mustachescripts/generate.sh
jonas-jonas
approved these changes
May 29, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #434
Summary by CodeRabbit
Chores
Infrastructure