feat: Add Grafana module #1509
feat: Add Grafana module #1509
Conversation
Implements Testcontainers support for Grafana with the following features: - Default Grafana image: grafana/grafana:11.0.0 - Support for custom username/password authentication - Anonymous access configuration - Ability to mount datasources, dashboards, plugins, and notifiers - HTTP health check wait strategy - Connection string generation with embedded credentials 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
✅ Deploy Preview for testcontainers-dotnet ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
HofmeisterAn
added
module
HofmeisterAn
force-pushed
the
develop
branch
3 times, most recently
from
4900ecd to
8fa5f1b
Compare
HofmeisterAn
left a comment
HofmeisterAn
left a comment
There was a problem hiding this comment.
Thanks for the PR! It looks great overall, but there are a few small things we need to fix before we can merge it.
Summary by CodeRabbit
WalkthroughAdds a new Grafana module and tests: new projects, builder/configuration/container types, docs, tests, global usings, and solution/CI wiring to integrate Grafana support and examples. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Dev as Developer
participant Builder as GrafanaBuilder
participant Config as GrafanaConfiguration
participant Container as GrafanaContainer
participant Docker as Docker Engine
Dev->>Builder: new GrafanaBuilder()
Note right of Builder `#DFF3E0`: Init sets image, port, creds, health-check
Dev->>Builder: WithUsername(u).WithPassword(p) / WithAnonymousAccessEnabled()
Dev->>Builder: Build()
Builder->>Config: create immutable configuration
Builder->>Container: new GrafanaContainer(config)
Container->>Docker: create & start container
Docker->>Container: mapped port & hostname
Container-->>Dev: GetBaseAddress() (http://host:port)
Dev->>Container: call Grafana API (with/without Basic auth)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
🔇 Additional comments (1)
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.
Actionable comments posted: 0
♻️ Duplicate comments (3)
src/Testcontainers.Grafana/GrafanaContainer.cs (1)
32-42: Reconsider the need for this method given authentication patterns.The
GetConnectionString()method embeds credentials in the URI, but this approach has several concerns:
- Security: Embedding credentials in URIs is problematic as they may be logged, cached, or exposed in stack traces.
- Grafana authentication: Modern Grafana typically uses API keys or Bearer tokens in HTTP headers, not HTTP Basic Auth via URI.
- Unused pattern: As noted in the previous review, tests authenticate using HTTP headers, suggesting this method may not align with actual usage patterns.
If this method isn't required by consumers or tests, consider removing it. Otherwise, clarify its intended use case.
If the method must remain, consider documenting its purpose and warning about security implications:
/// <summary> /// Gets the Grafana connection string. /// </summary> + /// <remarks> + /// This method embeds credentials in the URI for backward compatibility. + /// For production use, prefer API keys with the Authorization header. + /// Avoid logging or caching the returned connection string as it contains credentials. + /// </remarks> /// <returns>The Grafana connection string.</returns> public string GetConnectionString()Alternatively, if tests and typical usage rely on HTTP header authentication, remove this method:
- /// <summary> - /// Gets the Grafana connection string. - /// </summary> - /// <returns>The Grafana connection string.</returns> - public string GetConnectionString() - { - var endpoint = GetHttpEndpoint(); - var username = _configuration.Username ?? GrafanaBuilder.DefaultUsername; - var password = _configuration.Password ?? GrafanaBuilder.DefaultPassword; - return new UriBuilder(endpoint) - { - UserName = username, - Password = password - }.ToString(); - }src/Testcontainers.Grafana/GrafanaBuilder.cs (2)
83-116: Replace WithBindMount with WithResourceMapping for better compatibility.As noted in previous reviews, mounting shares with
WithBindMountcan cause incompatibility with remote container runtimes. The recommended approach is to useWithResourceMapping(FileInfo, FileInfo)to copy files into the container.Additionally, these provisioning methods lack test coverage. Consider adding tests or documenting the expected usage patterns.
Based on past review feedback.
133-133: Use WithAnonymousAccessDisabled() for consistency.As noted in previous reviews, line 133 should use the
WithAnonymousAccessDisabled()method instead of directly setting the environment variable.Apply this diff:
.WithUsername(DefaultUsername) .WithPassword(DefaultPassword) - .WithEnvironment("GF_AUTH_ANONYMOUS_ENABLED", "false") + .WithAnonymousAccessDisabled() .WithEnvironment("GF_AUTH_BASIC_ENABLED", "true")Based on past review feedback.
🧹 Nitpick comments (6)
tests/Testcontainers.Grafana.Tests/Testcontainers.Grafana.Tests.csproj (1)
3-3: Consider multi-targeting for broader test coverage.The test project targets only
net9.0, whereas the main Grafana project supportsnet8.0;net9.0;netstandard2.0;netstandard2.1. While this may be intentional for simplicity, testing against multiple frameworks ensures compatibility across the supported target frameworks.If broader coverage is desired, apply this diff:
- <TargetFrameworks>net9.0</TargetFrameworks> + <TargetFrameworks>net8.0;net9.0</TargetFrameworks>tests/Testcontainers.Grafana.Tests/GrafanaContainerTest.cs (5)
27-28: Consider removing ConfigureAwait(true).In test code,
ConfigureAwait(true)is redundant since that's the default behavior. Omitting it simplifies the code without changing functionality.Apply this diff to simplify:
- using var httpResponse = await httpClient.GetAsync("api/health", TestContext.Current.CancellationToken) - .ConfigureAwait(true); + using var httpResponse = await httpClient.GetAsync("api/health", TestContext.Current.CancellationToken);
46-47: Consider removing ConfigureAwait(true).Same as the previous test,
ConfigureAwait(true)is redundant in test code.Apply this diff:
- using var httpResponse = await httpClient.GetAsync("api/org", TestContext.Current.CancellationToken) - .ConfigureAwait(true); + using var httpResponse = await httpClient.GetAsync("api/org", TestContext.Current.CancellationToken);
53-65: Consider validating the URI format.While the current assertions verify that key components are present in the connection string, they don't validate that the connection string is a well-formed URI with credentials properly encoded.
Consider adding a URI parsing assertion:
public void GetConnectionStringReturnsExpectedFormat() { // When var connectionString = _grafanaContainer.GetConnectionString(); // Then Assert.NotNull(connectionString); + Assert.True(Uri.TryCreate(connectionString, UriKind.Absolute, out var uri), "Connection string should be a valid URI"); Assert.Contains(GrafanaBuilder.DefaultUsername, connectionString); Assert.Contains(GrafanaBuilder.DefaultPassword, connectionString); Assert.Contains(_grafanaContainer.Hostname, connectionString); }
97-98: Consider removing ConfigureAwait(true).Consistent with previous suggestions,
ConfigureAwait(true)is redundant in test code.Apply this diff:
- using var httpResponse = await httpClient.GetAsync("api/org", TestContext.Current.CancellationToken) - .ConfigureAwait(true); + using var httpResponse = await httpClient.GetAsync("api/org", TestContext.Current.CancellationToken);
131-132: Consider removing ConfigureAwait(true).Consistent with previous suggestions,
ConfigureAwait(true)is redundant in test code.Apply this diff:
- using var httpResponse = await httpClient.GetAsync("api/org", TestContext.Current.CancellationToken) - .ConfigureAwait(true); + using var httpResponse = await httpClient.GetAsync("api/org", TestContext.Current.CancellationToken);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
Testcontainers.sln(6 hunks)docs/modules/index.md(1 hunks)src/Testcontainers.Grafana/GrafanaBuilder.cs(1 hunks)src/Testcontainers.Grafana/GrafanaConfiguration.cs(1 hunks)src/Testcontainers.Grafana/GrafanaContainer.cs(1 hunks)src/Testcontainers.Grafana/Testcontainers.Grafana.csproj(1 hunks)src/Testcontainers.Grafana/Usings.cs(1 hunks)tests/Testcontainers.Grafana.Tests/.runs-on(1 hunks)tests/Testcontainers.Grafana.Tests/GrafanaContainerTest.cs(1 hunks)tests/Testcontainers.Grafana.Tests/Testcontainers.Grafana.Tests.csproj(1 hunks)tests/Testcontainers.Grafana.Tests/Usings.cs(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
tests/Testcontainers.Grafana.Tests/GrafanaContainerTest.cs (2)
src/Testcontainers.Grafana/GrafanaBuilder.cs (15)
GrafanaContainer(119-123)GrafanaBuilder(18-22)GrafanaBuilder(28-32)GrafanaBuilder(42-46)GrafanaBuilder(53-57)GrafanaBuilder(63-66)GrafanaBuilder(72-76)GrafanaBuilder(83-86)GrafanaBuilder(93-96)GrafanaBuilder(103-106)GrafanaBuilder(113-116)GrafanaBuilder(126-140)GrafanaBuilder(143-146)GrafanaBuilder(149-152)GrafanaBuilder(155-158)src/Testcontainers.Grafana/GrafanaContainer.cs (3)
GrafanaContainer(13-17)GetHttpEndpoint(23-26)GetConnectionString(32-42)
src/Testcontainers.Grafana/GrafanaBuilder.cs (2)
src/Testcontainers.Grafana/GrafanaConfiguration.cs (6)
PublicAPI(4-71)GrafanaConfiguration(12-18)GrafanaConfiguration(24-28)GrafanaConfiguration(34-38)GrafanaConfiguration(44-48)GrafanaConfiguration(55-60)src/Testcontainers.Grafana/GrafanaContainer.cs (2)
PublicAPI(4-43)GrafanaContainer(13-17)
src/Testcontainers.Grafana/GrafanaConfiguration.cs (2)
src/Testcontainers.Grafana/GrafanaBuilder.cs (1)
PublicAPI(4-159)src/Testcontainers.Grafana/GrafanaContainer.cs (1)
PublicAPI(4-43)
src/Testcontainers.Grafana/GrafanaContainer.cs (2)
src/Testcontainers.Grafana/GrafanaBuilder.cs (16)
PublicAPI(4-159)GrafanaContainer(119-123)GrafanaBuilder(18-22)GrafanaBuilder(28-32)GrafanaBuilder(42-46)GrafanaBuilder(53-57)GrafanaBuilder(63-66)GrafanaBuilder(72-76)GrafanaBuilder(83-86)GrafanaBuilder(93-96)GrafanaBuilder(103-106)GrafanaBuilder(113-116)GrafanaBuilder(126-140)GrafanaBuilder(143-146)GrafanaBuilder(149-152)GrafanaBuilder(155-158)src/Testcontainers.Grafana/GrafanaConfiguration.cs (6)
PublicAPI(4-71)GrafanaConfiguration(12-18)GrafanaConfiguration(24-28)GrafanaConfiguration(34-38)GrafanaConfiguration(44-48)GrafanaConfiguration(55-60)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: ci (Testcontainers, ubuntu-24.04)
- GitHub Check: analyze (csharp)
🔇 Additional comments (21)
tests/Testcontainers.Grafana.Tests/.runs-on (1)
1-1: LGTM!The runner specification is appropriate for Docker-based Grafana container tests.
src/Testcontainers.Grafana/Usings.cs (1)
1-8: LGTM!The global usings are appropriate for the Grafana module and follow the standard pattern used across other Testcontainers modules.
tests/Testcontainers.Grafana.Tests/Usings.cs (1)
1-6: LGTM!The global usings are appropriate for the Grafana test project.
src/Testcontainers.Grafana/Testcontainers.Grafana.csproj (1)
1-12: LGTM!The project configuration follows the standard pattern for Testcontainers modules with appropriate multi-targeting and dependencies.
docs/modules/index.md (1)
45-45: LGTM!The Grafana module entry is correctly formatted and positioned alphabetically in the modules table.
Testcontainers.sln (1)
65-66: LGTM!The solution file correctly adds both Grafana projects with proper build configurations and folder nesting, following the established solution structure.
Also applies to: 185-186, 365-368, 605-608, 812-812, 872-872
src/Testcontainers.Grafana/GrafanaContainer.cs (2)
9-17: LGTM!The constructor follows the standard pattern for Testcontainers modules.
19-26: LGTM!The
GetHttpEndpoint()method correctly constructs the Grafana HTTP endpoint using the mapped port.tests/Testcontainers.Grafana.Tests/GrafanaContainerTest.cs (3)
3-16: LGTM!The test class setup correctly implements IAsyncLifetime for proper container lifecycle management. The container is appropriately shared across tests in this class.
67-83: LGTM!The nested test class properly isolates custom credential testing with its own container instance. The IAsyncLifetime implementation ensures proper resource cleanup.
105-120: LGTM!The nested test class properly validates anonymous access configuration with its own isolated container instance.
src/Testcontainers.Grafana/GrafanaConfiguration.cs (4)
12-18: LGTM!The basic constructor correctly initializes the configuration with optional username and password parameters.
55-60: LGTM!The merge constructor correctly implements the immutable copy pattern, preferring new values when present and falling back to old values when null.
65-70: LGTM!The get-only properties enforce immutability as expected.
24-38: ****The review comment incorrectly identifies a credential loss issue. The code properly preserves
UsernameandPasswordduring cloning.When
Clone()is called, it creates a newGrafanaConfigurationvia the copy constructors (lines 24-28 or 34-38), which intentionally have null properties. This is then passed to theMergemethod, which invokes the merge constructor (lines 54-62). The merge constructor explicitly preserves credentials from the old configuration using null-coalescing operators:Username = newValue.Username ?? oldValue.Username; Password = newValue.Password ?? oldValue.Password;Since
DockerResourceConfigurationcontains the previously configured credentials and serves asoldValuein the merge operation, credentials are retained when cloning. This pattern is consistent across all container modules in the codebase, confirming it is intentional framework design.Likely an incorrect or invalid review comment.
src/Testcontainers.Grafana/GrafanaBuilder.cs (6)
18-32: LGTM!The constructors correctly implement the builder pattern with proper initialization.
42-57: LGTM!The credential configuration methods correctly implement the fluent, immutable builder pattern and set appropriate Grafana environment variables.
72-76: LGTM! Note the admin role for anonymous access.The anonymous access methods are correctly implemented. Note that enabling anonymous access grants Admin role to anonymous users, which is appropriate for testing scenarios but should be documented for production use.
119-123: LGTM!The Build method correctly validates configuration before constructing the container.
143-158: LGTM!The Clone and Merge methods correctly implement the immutable builder pattern. Note that the Clone methods may not preserve Username/Password properties, which was flagged in the GrafanaConfiguration.cs review.
7-7: ---Update Grafana image to 12.3.0 or confirm 11.0.0 is required.
The image is pinned to version 11.0.0, but the latest stable version is 12.3.0. If there's no specific reason to remain on 11.0.0 (e.g., testing backward compatibility), consider updating to the current stable release.
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
docs/modules/grafana.md(1 hunks)docs/modules/index.md(1 hunks)docs/modules/pulsar.md(2 hunks)mkdocs.yml(1 hunks)src/Testcontainers.Grafana/.editorconfig(1 hunks)src/Testcontainers.Grafana/GrafanaBuilder.cs(1 hunks)src/Testcontainers.Grafana/GrafanaConfiguration.cs(1 hunks)src/Testcontainers.Grafana/GrafanaContainer.cs(1 hunks)src/Testcontainers.Grafana/Testcontainers.Grafana.csproj(1 hunks)src/Testcontainers.Grafana/Usings.cs(1 hunks)tests/Testcontainers.Grafana.Tests/.editorconfig(1 hunks)tests/Testcontainers.Grafana.Tests/GrafanaContainerTest.cs(1 hunks)tests/Testcontainers.Grafana.Tests/Testcontainers.Grafana.Tests.csproj(1 hunks)tests/Testcontainers.Grafana.Tests/Usings.cs(1 hunks)
✅ Files skipped from review due to trivial changes (3)
- src/Testcontainers.Grafana/.editorconfig
- docs/modules/grafana.md
- mkdocs.yml
🚧 Files skipped from review as they are similar to previous changes (5)
- src/Testcontainers.Grafana/Testcontainers.Grafana.csproj
- tests/Testcontainers.Grafana.Tests/Testcontainers.Grafana.Tests.csproj
- src/Testcontainers.Grafana/GrafanaContainer.cs
- src/Testcontainers.Grafana/Usings.cs
- src/Testcontainers.Grafana/GrafanaConfiguration.cs
🧰 Additional context used
🧬 Code graph analysis (2)
src/Testcontainers.Grafana/GrafanaBuilder.cs (2)
src/Testcontainers.Grafana/GrafanaConfiguration.cs (6)
PublicAPI(4-71)GrafanaConfiguration(12-18)GrafanaConfiguration(24-28)GrafanaConfiguration(34-38)GrafanaConfiguration(44-48)GrafanaConfiguration(55-60)src/Testcontainers.Grafana/GrafanaContainer.cs (2)
PublicAPI(4-27)GrafanaContainer(13-17)
tests/Testcontainers.Grafana.Tests/GrafanaContainerTest.cs (2)
src/Testcontainers.Grafana/GrafanaBuilder.cs (10)
GrafanaContainer(70-74)GrafanaBuilder(18-22)GrafanaBuilder(28-32)GrafanaBuilder(42-46)GrafanaBuilder(53-57)GrafanaBuilder(63-67)GrafanaBuilder(77-86)GrafanaBuilder(89-92)GrafanaBuilder(95-98)GrafanaBuilder(101-104)src/Testcontainers.Grafana/GrafanaContainer.cs (2)
GrafanaContainer(13-17)GetBaseAddress(23-26)
🪛 LanguageTool
docs/modules/index.md
[style] ~46-~46: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: .../Testcontainers.Grafana) | Source | | InfluxDB | `influxd...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: analyze (csharp)
🔇 Additional comments (7)
tests/Testcontainers.Grafana.Tests/.editorconfig (1)
1-1: Verify EditorConfig placement and hierarchy.Placing
.editorconfigwithroot = trueat the test project level is unconventional. Typically,.editorconfigresides at the solution root to establish coding standards across all projects. Settingroot = truehere will prevent EditorConfig from discovering parent configuration files, which could bypass solution-wide style guidelines.Verify whether this file should instead inherit from a solution-level
.editorconfig(by removingroot = trueor deleting this file entirely) or whether test projects intentionally require isolated configuration.tests/Testcontainers.Grafana.Tests/Usings.cs (1)
1-9: LGTM!The global using directives are appropriate for a Testcontainers test project testing HTTP-based services like Grafana. The namespaces cover HTTP operations, async testing, the Testcontainers library, and the Xunit test framework.
docs/modules/pulsar.md (1)
51-51: Verify that Pulsar documentation changes belong in this PR.The additions of
newkeyword before builder instantiation improve code clarity and are stylistically correct. However, these changes appear unrelated to the Grafana module additions described in the PR objectives. Please confirm whether these updates should be part of this PR or addressed in a separate commit.Also applies to: 73-73
tests/Testcontainers.Grafana.Tests/GrafanaContainerTest.cs (1)
18-31: LGTM!The async lifetime pattern is correctly implemented with proper disposal semantics.
src/Testcontainers.Grafana/GrafanaBuilder.cs (3)
42-57: LGTM!The credential configuration methods correctly merge the configuration state and set the corresponding Grafana environment variables.
77-86: LGTM!The initialization correctly configures the default Grafana instance with appropriate wait strategy targeting the health endpoint.
63-67: No changes needed—Grafana supports simultaneous anonymous and admin access by design.The code is correct. Grafana supports anonymous access via config/env vars while the default/admin account still exists, and anonymous users get the org/role you configure. Anonymous mode simply allows unauthenticated access with the role you set; it does not remove or disable the admin account. Credentials should remain configured; the
WithAnonymousAccessEnabled()method does not need to clear them.
2 participants
Implements Testcontainers support for Grafana with the following features: