Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 16 additions & 9 deletions .github/pull_request_template.md
Original file line number Diff line number Diff line change
@@ -1,50 +1,57 @@
# Pull Request

<!--
Thanks for contributing to Valkey GLIDE!

Please make sure you are aware of our [contributing guidelines](https://github.com/valkey-io/valkey-glide-csharp/blob/main/CONTRIBUTING.md).
-->

### Summary
## Summary

<!--
Add a summary describing the changes.
-->

### Issue Link
## Issue Link

<!--
Link to the corresponding GitHub issue. Please raise an issue if none exists.
Use "Closes" to automatically close the issue when this pull request is merged.
-->

This pull request is linked to issue: [REPLACE ME]
Closes #[GITHUB-ISSUE-ID].
Comment thread
currantw marked this conversation as resolved.

### Features and Behaviour Changes
## Features and Behaviour Changes

<!--
Outline the feature support and behaviour changes included in this pull request.
-->

### Implementation
## Implementation

<!--
Describe the implementation details. Highlight key code changes and call out any areas where you want reviewers to pay extra attention.
-->

### Limitations
## Limitations

<!--
Describe any features or use cases that are not implemented or are only partially supported.
-->

### Testing
## Testing

<!--
Describe what tests have been conducted and any relevant test results.
-->

### Related Issues
## Related Issues

<!--
List related issues and pull request such closed pull requests and open issues.
-->

### Checklist
## Checklist

<!--
Before submitting the PR make sure the following are checked.
Expand Down
11 changes: 11 additions & 0 deletions .markdownlint-cli2.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
gitignore: true

config:
# Line length — not auto-fixable.
MD013: false
# Duplicate headings — allow under different parents only.
MD024:
siblings_only: true

ignores:
- "valkey-glide/**"
6 changes: 3 additions & 3 deletions AGENTS.md
Original file line number Diff line number Diff line change
Expand Up @@ -45,9 +45,9 @@ Common commands:

- Prefer `task` commands for linting and formatting.
- Lint checks (read-only, fail on issues):
- `task lint` (all checks), `task lint:rust`, `task lint:csharp`, `task lint:yaml`, `task lint:actions`
- `task lint` (all checks), `task lint:rust`, `task lint:csharp`, `task lint:yaml`, `task lint:actions`, `task lint:markdown`
- Auto-fix formatting:
- `task format` (all languages), `task format:rust`, `task format:csharp`, `task format:yaml`
- `task format` (all languages), `task format:rust`, `task format:csharp`, `task format:yaml`, `task format:markdown`
- Link checking (separate from lint, slower):
- `task check-links`

Expand Down Expand Up @@ -163,7 +163,7 @@ Note: Conventional Commits apply to commit messages only. Do not enforce this fo
## Quality Gates (Agent Checklist)

- Build passes on `net8.0`.
- Lint passes: `task lint` (or individual `task lint:rust`, `task lint:csharp`, `task lint:yaml`).
- Lint passes: `task lint` (or individual `task lint:rust`, `task lint:csharp`, `task lint:yaml`, `task lint:markdown`).
- Tests pass; targeted via filters instead of per-file execution.
- Generated outputs not committed.
- Public API changes respect StackExchange.Redis compatibility.
Expand Down
13 changes: 8 additions & 5 deletions DEVELOPER.md
Original file line number Diff line number Diff line change
Expand Up @@ -309,7 +309,8 @@ You can combine this with test filter as well:
```bash
cluster-endpoints=localhost:7000 standalone-endpoints=localhost:6379 tls=true dotnet test --logger "console;verbosity=detailed" --filter "FullyQualifiedName~GetReturnsNull"
```
#### IAM Authentication Tests

### IAM Authentication Tests

To run [IAM authentication tests](tests/Valkey.Glide.IntegrationTests/IamAuthTests.cs) locally, set the following environment variables:

Expand Down Expand Up @@ -355,10 +356,11 @@ Before making a contribution, ensure that all new user APIs and non-obvious code
task lint

# Run linters for specific languages:
task lint:rust # Run Rust linting
task lint:csharp # Run C# linting
task lint:yaml # Run YAML linting
task lint:actions # Run GitHub Actions linting
task lint:rust # Run Rust linting
task lint:csharp # Run C# linting
task lint:yaml # Run YAML linting
task lint:actions # Run GitHub Actions linting
task lint:markdown # Run Markdown linting

# Run all formatters:
task format
Expand All @@ -367,6 +369,7 @@ task format
task format:rust
task format:csharp
task format:yaml
task format:markdown

# Check for broken links
task check-links
Expand Down
59 changes: 37 additions & 22 deletions Taskfile.yml
Original file line number Diff line number Diff line change
Expand Up @@ -146,18 +146,25 @@ tasks:

build:
desc: Build the solution
cmds:
- dotnet build {{.TARGET_PATH}} --configuration Release
cmd: dotnet build {{.TARGET_PATH}} --configuration Release
Comment thread
currantw marked this conversation as resolved.

test:
desc: Build and run all tests
deps: [build]
cmds:
- task: test:coverage:all

# Lint
# --------------------------------------------------------------------------------

lint:
desc: "Run all linters"
deps: [lint:rust, lint:csharp, lint:yaml, lint:actions]
deps:
- lint:rust
- lint:csharp
- lint:yaml
- lint:actions
- lint:markdown

lint:rust:
desc: "Run Rust linting"
Expand All @@ -178,43 +185,51 @@ tasks:

lint:yaml:
desc: "Run YAML linting"
cmds:
- npx prettier --check "**/*.yml"
Comment thread
currantw marked this conversation as resolved.
cmd: npx prettier --check "**/*.yml"

lint:actions:
desc: "Run GitHub Actions linting"
cmds:
- actionlint
cmd: actionlint

lint:markdown:
desc: "Run Markdown linting"
cmd: npx markdownlint-cli2 "**/*.md"

# Format
# --------------------------------------------------------------------------------

format:
desc: "Run all formatters"
cmds:
- task: format:rust
- task: format:csharp
- task: format:yaml
deps:
- format:rust
- format:csharp
- format:yaml
- format:markdown
Comment thread
currantw marked this conversation as resolved.

format:rust:
desc: "Run Rust formatting"
dir: rust
cmds:
- cargo fmt --all
cmd: cargo fmt --all

format:csharp:
desc: "Run C# formatting"
cmds:
- dotnet format
cmd: dotnet format

format:yaml:
desc: "Run YAML formatting"
cmds:
- npx prettier --write "**/*.yml"
cmd: npx prettier --write "**/*.yml"

format:markdown:
desc: "Run Markdown formatting"
cmd: npx markdownlint-cli2 --fix "**/*.md"

# Additional checks
# --------------------------------------------------------------------------------

check-links:
desc: "Check for broken links"
cmds:
- lychee --config lychee.toml .
cmd: lychee --config lychee.toml .

check-examples:
desc: "Extract and validate C# code examples in doc comments"
cmds:
- python3 scripts/check_examples.py
desc: "Check C# examples in doc comments"
cmd: python3 scripts/check_examples.py
2 changes: 1 addition & 1 deletion docs/ci-cd.md
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ a set of platforms, versions, and tests to be run. The json files list our avail
## Files

| File | Purpose |
|------|---------|
| ---- | ------- |
| `os-matrix.json` | OS/runner platforms (VMs and containers) |
| `server-matrix.json` | Valkey and Redis server versions |
| `version-matrix.json` | .NET SDK versions |
Expand Down
13 changes: 12 additions & 1 deletion docs/configuration-architecture-analysis.md
Comment thread
currantw marked this conversation as resolved.
Original file line number Diff line number Diff line change
Expand Up @@ -7,18 +7,20 @@ This document analyzes the configuration architecture in the Valkey.Glide C# cli
## Configuration Classes Relationship

### ConfigurationOptions

- **Purpose**: External API configuration class that follows StackExchange.Redis compatibility patterns
- **Location**: `sources/Valkey.Glide/Abstract/ConfigurationOptions.cs`
- **Role**: User-facing configuration interface

### ConnectionConfiguration

- **Purpose**: Internal configuration classes that map to the underlying FFI layer
- **Location**: `sources/Valkey.Glide/ConnectionConfiguration.cs`
- **Role**: Internal configuration representation and builder pattern implementation

## Configuration Flow

```
```text
ConfigurationOptions → ClientConfigurationBuilder → ConnectionConfig → FFI.ConnectionConfig
```

Expand Down Expand Up @@ -53,10 +55,12 @@ internal static T CreateClientConfigBuilder<T>(ConfigurationOptions configuratio
### Configuration Builders

The builder pattern is implemented through:

- `StandaloneClientConfigurationBuilder` (line 525)
- `ClusterClientConfigurationBuilder` (line 550)

Both inherit from `ClientConfigurationBuilder<T>` which provides:

- Fluent API methods (`WithXxx()`)
- Property setters
- Internal `ConnectionConfig Build()` method
Expand Down Expand Up @@ -92,6 +96,7 @@ internal ConfigurationOptions RawConfig { private set; get; }
## Potential Configuration Change Approaches

### 1. Connection Recreation (Current Pattern)

```csharp
// Current approach - requires new connection
var newConfig = oldConfig.Clone();
Expand Down Expand Up @@ -136,12 +141,15 @@ public async Task<bool> TryUpdateConfigurationAsync<T>(Action<T> configure)
## ReadFrom Configuration Specifics

### Current Implementation

- `ReadFrom` is a struct (line 74) with `ReadFromStrategy` enum and optional AZ string
- Mapped in `CreateClientConfigBuilder()` at line 199
- Flows through to FFI layer via `ConnectionConfig.ToFfi()` method

### ReadFrom Change Requirements

To change `ReadFrom` configuration at runtime would require:

1. **API Design**: Method to accept new `ReadFrom` configuration
2. **Validation**: Ensure new configuration is compatible with current connection type
3. **FFI Updates**: Update the underlying client configuration
Expand All @@ -150,15 +158,18 @@ To change `ReadFrom` configuration at runtime would require:
## Recommendations

### Short Term

1. **Document Current Limitations**: Clearly document that configuration changes require connection recreation
2. **Helper Methods**: Provide utility methods for common reconfiguration scenarios:

```csharp
public static async Task<ConnectionMultiplexer> RecreateWithReadFromAsync(
ConnectionMultiplexer current,
ReadFrom newReadFrom)
```

### Long Term

1. **Runtime Reconfiguration API**: Implement selective runtime configuration updates for non-disruptive changes
2. **Configuration Validation**: Add validation to determine which changes require reconnection vs. runtime updates
3. **Connection Pool Management**: Consider connection pooling to minimize disruption during reconfiguration
Expand Down
Loading