-
Notifications
You must be signed in to change notification settings - Fork 9
feat: Add C# SDK #140
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
base: main
Are you sure you want to change the base?
feat: Add C# SDK #140
Conversation
- Add C# code generator in internal/gen/csharp/ - Add C# SDK in sdk/csharp/ with Pipeline, Environment, and Schema types - Support for CommandStep, BlockStep, InputStep, WaitStep, TriggerStep, GroupStep - Includes 26 passing unit tests - Targets .NET 8.0 and .NET 9.0 - Nx integration via project.json Amp-Thread-ID: https://ampcode.com/threads/T-019b9fab-5a9e-701d-80a7-b75151077c5b
- Add apps/csharp example application - Add C# to CI pipeline (ci/src/pipeline.ts) and release pipeline (ci/src/release.ts) - Add NUGET_API_KEY for NuGet publishing - Add DocFX documentation setup (docfx.json, filterConfig.yml, index.md, toc.yml) - Add WaitStepTests.cs for complete test coverage - Add install, docs:build, docs:serve targets to sdk/csharp/project.json - Update .gitignore for C# build artifacts (bin/, obj/) Amp-Thread-ID: https://ampcode.com/threads/T-019ba0f4-7bb7-701a-a467-9a5f507df553
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 27048cd887
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
- Add Image property to Pipeline and propagate in SetPipeline/Build - Change Agents type from AgentsObject to object? to support both AgentsObject (map) and AgentsList (list) forms per Buildkite schema - Add SetAgents() overloads for both forms - Add SetImage() helper method - Add HasAgents() helper for proper null/empty checking Amp-Thread-ID: https://ampcode.com/threads/T-019ba0f4-7bb7-701a-a467-9a5f507df553
|
@codex review again please! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6c48b6ad83
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
- Add CSharp() and CSharpType() methods to Value interface - Implement CSharp generation for String, Number, Boolean, Array, Object, Enum, Union, PropertyReference types - Add GenerateCSharpPipelineSchema() to PipelineSchemaGenerator - Add ToTitleCase() to utils package - Add System.Collections.Generic to C# file usings - Simplify union types to use 'object' for now - Fix enum fallback to use 'string' when values can't be C# enums
The C# types are handwritten (not auto-generated) so the target just outputs a message and exits successfully like Ruby. Amp-Thread-ID: https://ampcode.com/threads/T-019baef5-42c2-76ac-9db4-4be0c6931088
Replace handwritten wrapper classes (DependsOn, SoftFail, Skip, Plugin) with reusable generic union types: - OneOrMany<T>: for string | string[] patterns - BoolOr<T>: for bool | T patterns - StringOr<T>: for string | T patterns - OneOf<T1,T2> and OneOf<T1,T2,T3>: for other union types Add System.Text.Json and YamlDotNet converters for serialization. Benefits: - Implicit operators allow natural assignment without explicit conversions - Type-safe union handling replaces object? everywhere - ~200 lines of generic code replaces ~1k lines of wrappers - Follows Stripe .NET SDK patterns Amp-Thread-ID: https://ampcode.com/threads/T-019bb507-73d1-73c2-a916-adde81b6822d
|
@codex this has been substantially refactored to use union types better, can you dig in and deeply review please? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bea510a16c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
The union primitives added +487 lines for minimal benefit in a write-only SDK. Reverting to the simpler approach: - Use object? for union fields with XML doc comments - Keep small wrapper classes (DependsOn, SoftFail, Skip, Plugin) - Rely on default JSON/YAML serialization behavior This matches the SDK's primary use case of generating pipeline YAML/JSON without the overhead of custom converters. Amp-Thread-ID: https://ampcode.com/threads/T-019bb507-73d1-73c2-a916-adde81b6822d
The Plugin class had a Value property that wouldn't serialize to the correct Buildkite format. Users should pass the correct shapes directly to the object? properties. Added XML doc examples showing the expected formats for both Plugins and Secrets properties.
HugeIRL
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lox had a review here and things look great! All tests pass locally and with the example c# program.
Just a few things you may want to consider but shouldn't be merge blocking:
- No test coverage for Plugins/Secrets: There are no tests validating that plugins or secrets serialize correctly.
Here's a rough example to consider:
[Test]
public void CommandStep_WithPlugins_SerializesCorrectly()
{
var step = new CommandStep
{
Plugins = new object[] {
"docker#v5.0.0",
new Dictionary<string, object> {
["ecr#v2.0.0"] = new { login = true }
}
}
};
// Assert serialization matches expected format
}-
Documentation examples use C# 12 syntax: The examples use collection expressions which require C# 12. If the SDK targets older versions, users might find the examples confusing.
-
Should there be runtime validation? Since Plugins accepts
object?, invalid types will only fail at serialization time. Is that acceptable, or should there be validation?
Here's an example:
// All of these may compile successfully, even though some are wrong:
new CommandStep
{
Plugins = "docker#v5.0.0", // ✅ Valid
Plugins = new object[] { "docker#v5.0.0" }, // ✅ Valid
Plugins = new Dictionary<string, object> { ["ecr"] = new { login = true } }, // ✅ Valid
Plugins = 42, // ❌ Wrong, but compiles!
Plugins = new { foo = "bar" }, // ❌ Wrong, but compiles!
Plugins = new List<int> { 1, 2, 3 }, // ❌ Wrong, but compiles!
}
Summary
Adds a C# SDK for defining Buildkite pipelines programmatically. Targets .NET 8.0 and .NET 9.0.
Usage
What's included
apps/csharp/