Open
Conversation
…og uploads. Add tests.
Member
|
👀 |
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a new Elastic.Documentation.Integrations service project with S3 utilities (IS3EtagCalculator, S3EtagCalculator, S3IncrementalUploader) and tests. Introduces ChangelogUploadService and a CLI subcommand Sequence Diagram(s)sequenceDiagram
participant CLI as ChangelogCommand
participant Service as ChangelogUploadService
participant FS as FileSystem
participant Uploader as S3IncrementalUploader
participant Calculator as S3EtagCalculator
participant S3 as AWS S3
CLI->>Service: Upload(args)
Service->>FS: Resolve & verify changelog directory
Service->>FS: Scan directory for *.yaml/*.yml
Service->>FS: Read & deserialize files
Service->>Service: Extract & validate product names
Service->>Uploader: Upload(targets)
loop For each target
Uploader->>S3: GetObjectMetadataAsync(bucket, key)
alt Object exists
S3-->>Uploader: Remote ETag
else Object not found
S3-->>Uploader: NotFound
end
Uploader->>Calculator: CalculateS3ETag(localPath)
Calculator->>FS: Read file (parts if large)
Calculator-->>Uploader: Local ETag
alt Local ETag ≠ Remote ETag
Uploader->>S3: PutObjectAsync(file)
S3-->>Uploader: Success or Error
Uploader->>Uploader: Increment uploaded/failed
else ETags match
Uploader->>Uploader: Increment skipped
end
end
Uploader-->>Service: UploadResult
Service-->>CLI: Return status
Suggested labels
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches✨ Simplify code
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
tests/Elastic.Changelog.Tests/Uploading/ChangelogUploadServiceTests.cs (2)
9-9: Unused import.
Elastic.Documentation.Integrations.S3appears unused in this file. TheUploadTargettype is referenced but it's returned fromDiscoverUploadTargets, not directly imported from this namespace.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/Elastic.Changelog.Tests/Uploading/ChangelogUploadServiceTests.cs` at line 9, Remove the unused using directive for Elastic.Documentation.Integrations.S3 from the test file; the UploadTarget type is obtained via DiscoverUploadTargets and not referenced directly here, so delete the using line `using Elastic.Documentation.Integrations.S3;` to clean up the import (if you later need UploadTarget directly, either reference it via its declaring namespace or add the using at that time).
29-29: UnnecessaryGC.SuppressFinalizecall.Per coding guidelines,
GC.SuppressFinalizeshould only be used in Dispose methods when the class has a finalizer or manages unmanaged resources. This class has neither. IfIDisposableis required for the test fixture, the Dispose body can be empty.Suggested fix
- public void Dispose() => GC.SuppressFinalize(this); + public void Dispose() { }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/Elastic.Changelog.Tests/Uploading/ChangelogUploadServiceTests.cs` at line 29, Remove the unnecessary GC.SuppressFinalize call from the Dispose method in the ChangelogUploadServiceTests class: update the Dispose() implementation to either be an empty method body (if the test fixture must implement IDisposable) or remove the Dispose method and IDisposable implementation entirely if not required; ensure you modify the Dispose() method signature accordingly and do not introduce any finalizers or unmanaged resource handling.src/tooling/docs-builder/Commands/ChangelogCommand.cs (1)
1310-1316: Public async method missingAsyncsuffix.Per coding guidelines, public async methods should use
Asyncsuffix (e.g.,UploadAsync). However, I notice other subcommand methods in this file (Create,Bundle,Remove, etc.) also don't follow this convention, so this appears to be an established pattern for CLI command handlers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tooling/docs-builder/Commands/ChangelogCommand.cs` around lines 1310 - 1316, The public async method Upload should be renamed to UploadAsync to follow the async-suffix convention: rename the method symbol Upload to UploadAsync, update all internal references and call sites to use UploadAsync, and if external consumers or reflection/CLI wiring depends on the old name keep a short obsolete wrapper method Upload that delegates to UploadAsync to preserve compatibility; check related command handler methods like Create, Bundle, Remove for consistency and update tests or registration code that invoke Upload to the new UploadAsync symbol.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/services/Elastic.Changelog/Uploading/ChangelogUploadService.cs`:
- Around line 87-89: The AmazonS3Client instance (s3Client) is created but never
disposed, which can leak HTTP connections; change the creation to a using
declaration or wrap it in a using block so the AmazonS3Client is disposed after
use, e.g. create s3Client with using and then construct
S3IncrementalUploader(logFactory, s3Client, _fileSystem, args.S3BucketName) and
await uploader.Upload(targets, ctx) inside that scope; ensure the variable names
AmazonS3Client/s3Client, S3IncrementalUploader/uploader and the call to
Upload(...) are updated to live inside the using scope so disposal happens even
on exceptions.
In `@tests/Elastic.Documentation.Integrations.Tests/S3/S3EtagCalculatorTests.cs`:
- Around line 57-67: The test currently only checks determinism; to prove
caching, call _calculator.CalculateS3ETag(path, ct) once, then modify the file
contents via _fileSystem (e.g., overwrite or replace MockFileData at path) and
call CalculateS3ETag(path, ct) again, asserting the second result equals the
first; reference the CalculateS3ETag method, the _calculator instance, the path
produced by TempPath("cached.yaml"), and the in-memory _fileSystem/MockFileData
to perform the mutation so the test fails if the calculator re-reads instead of
returning a cached value.
---
Nitpick comments:
In `@src/tooling/docs-builder/Commands/ChangelogCommand.cs`:
- Around line 1310-1316: The public async method Upload should be renamed to
UploadAsync to follow the async-suffix convention: rename the method symbol
Upload to UploadAsync, update all internal references and call sites to use
UploadAsync, and if external consumers or reflection/CLI wiring depends on the
old name keep a short obsolete wrapper method Upload that delegates to
UploadAsync to preserve compatibility; check related command handler methods
like Create, Bundle, Remove for consistency and update tests or registration
code that invoke Upload to the new UploadAsync symbol.
In `@tests/Elastic.Changelog.Tests/Uploading/ChangelogUploadServiceTests.cs`:
- Line 9: Remove the unused using directive for
Elastic.Documentation.Integrations.S3 from the test file; the UploadTarget type
is obtained via DiscoverUploadTargets and not referenced directly here, so
delete the using line `using Elastic.Documentation.Integrations.S3;` to clean up
the import (if you later need UploadTarget directly, either reference it via its
declaring namespace or add the using at that time).
- Line 29: Remove the unnecessary GC.SuppressFinalize call from the Dispose
method in the ChangelogUploadServiceTests class: update the Dispose()
implementation to either be an empty method body (if the test fixture must
implement IDisposable) or remove the Dispose method and IDisposable
implementation entirely if not required; ensure you modify the Dispose() method
signature accordingly and do not introduce any finalizers or unmanaged resource
handling.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 12eb5656-ac7a-4b25-ab44-6f14ecf03f6f
📒 Files selected for processing (15)
docs-builder.slnxsrc/services/Elastic.Changelog/Elastic.Changelog.csprojsrc/services/Elastic.Changelog/Uploading/ChangelogUploadService.cssrc/services/Elastic.Documentation.Assembler/Deploying/IncrementalDeployService.cssrc/services/Elastic.Documentation.Assembler/Deploying/Synchronization/AwsS3SyncPlanStrategy.cssrc/services/Elastic.Documentation.Assembler/Elastic.Documentation.Assembler.csprojsrc/services/Elastic.Documentation.Integrations/Elastic.Documentation.Integrations.csprojsrc/services/Elastic.Documentation.Integrations/S3/S3EtagCalculator.cssrc/services/Elastic.Documentation.Integrations/S3/S3IncrementalUploader.cssrc/tooling/docs-builder/Commands/ChangelogCommand.cstests-integration/Elastic.Assembler.IntegrationTests/DocsSyncTests.cstests/Elastic.Changelog.Tests/Uploading/ChangelogUploadServiceTests.cstests/Elastic.Documentation.Integrations.Tests/Elastic.Documentation.Integrations.Tests.csprojtests/Elastic.Documentation.Integrations.Tests/S3/S3EtagCalculatorTests.cstests/Elastic.Documentation.Integrations.Tests/S3/S3IncrementalUploaderTests.cs
src/services/Elastic.Changelog/Uploading/ChangelogUploadService.cs
Outdated
Show resolved
Hide resolved
tests/Elastic.Documentation.Integrations.Tests/S3/S3EtagCalculatorTests.cs
Show resolved
Hide resolved
…atorTests.cs Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
…to changelog-upload-adjustments
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/services/Elastic.Changelog/Uploading/ChangelogUploadService.cs`:
- Around line 141-144: The LINQ projection on entry.Products currently
dereferences p.Product directly, which can throw if any product element is null;
update the chain in ChangelogUploadService (the expression starting with
entry.Products.Select(...).Where(...).ToList()) to first filter out null product
items (e.g., Where(p => p != null && !string.IsNullOrWhiteSpace(p.Product)) or
use pattern matching like Where(p => p is { Product: var prod } &&
!string.IsNullOrWhiteSpace(prod)) before projecting Select(p => p.Product) so
you don't need the null-forgiving operator and valid products aren't skipped.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7537a0c5-3d95-4059-aa1e-dbcce8962fa7
📒 Files selected for processing (2)
src/services/Elastic.Changelog/Uploading/ChangelogUploadService.cstests/Elastic.Documentation.Integrations.Tests/S3/S3EtagCalculatorTests.cs
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/Elastic.Documentation.Integrations.Tests/S3/S3EtagCalculatorTests.cs
src/services/Elastic.Changelog/Uploading/ChangelogUploadService.cs
Outdated
Show resolved
Hide resolved
…e.cs Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
* Fix redirect tests * Remove changelog redirects implemented elsewhere
) Elasticsearch Serverless now defaults semantic_text to Jina, making the explicit Jina sub-fields redundant and the ELSER inference ID unnecessary. This removes both inference ID constants, all .jina field mappings, and lets semantic_text fields use the platform default. Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Made with ❤️️ by updatecli Co-authored-by: elastic-observability-automation[bot] <180520183+elastic-observability-automation[bot]@users.noreply.github.com>
The deploy apply command used RealRead which lacks AllowedSpecialFolder.Temp, causing ScopedFileSystemException when AwsS3SyncApplyStrategy stages files in /tmp/ for S3 upload. Switch to RealWrite which permits temp directory access. Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/services/Elastic.Documentation.Integrations/S3/S3IncrementalUploader.cs`:
- Around line 58-62: The per-target catch in S3IncrementalUploader is currently
catching all exceptions and treating cancellations as failures; change the
handler so cancellation exceptions (OperationCanceledException and
TaskCanceledException) are rethrown immediately (or propagated) before
logging/incrementing failed, leaving other exceptions to be logged with
_logger.LogError("Failed to upload {LocalPath} → s3://{Bucket}/{S3Key}",
target.LocalPath, bucketName, target.S3Key) and failed++ as before; locate the
catch block around the upload of each target in S3IncrementalUploader.cs and add
the cancellation check/throw at the top of the catch.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 44eaf065-b0f3-4ba7-839a-c33597004d88
📒 Files selected for processing (9)
src/services/Elastic.Changelog/Uploading/ChangelogUploadService.cssrc/services/Elastic.Documentation.Assembler/Deploying/IncrementalDeployService.cssrc/services/Elastic.Documentation.Assembler/Elastic.Documentation.Assembler.csprojsrc/services/Elastic.Documentation.Integrations/S3/S3EtagCalculator.cssrc/services/Elastic.Documentation.Integrations/S3/S3IncrementalUploader.cssrc/tooling/docs-builder/Commands/ChangelogCommand.cstests-integration/Elastic.Assembler.IntegrationTests/DocsSyncTests.cstests/Elastic.Changelog.Tests/Uploading/ChangelogUploadServiceTests.cstests/Elastic.Documentation.Integrations.Tests/S3/S3IncrementalUploaderTests.cs
✅ Files skipped from review due to trivial changes (4)
- tests-integration/Elastic.Assembler.IntegrationTests/DocsSyncTests.cs
- src/services/Elastic.Documentation.Assembler/Elastic.Documentation.Assembler.csproj
- src/services/Elastic.Documentation.Assembler/Deploying/IncrementalDeployService.cs
- src/services/Elastic.Documentation.Integrations/S3/S3EtagCalculator.cs
🚧 Files skipped from review as they are similar to previous changes (1)
- src/tooling/docs-builder/Commands/ChangelogCommand.cs
src/services/Elastic.Documentation.Integrations/S3/S3IncrementalUploader.cs
Outdated
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/services/Elastic.Documentation.Integrations/S3/S3IncrementalUploader.cs`:
- Line 32: Rename the public async method Upload to UploadAsync to follow
PascalCase+Async naming; update the method declaration in S3IncrementalUploader
(public async Task<UploadResult> Upload(IReadOnlyList<UploadTarget> targets,
Cancel ctx = default)) to public async Task<UploadResult>
UploadAsync(IReadOnlyList<UploadTarget> targets, Cancel ctx = default) and then
update all call sites, interface/abstract declarations, and any overrides that
reference Upload to call/implement UploadAsync instead (including references to
UploadResult, UploadTarget, and Cancel types).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 89c254a8-ac83-444e-a2ac-cbb4a68033ce
📒 Files selected for processing (1)
src/services/Elastic.Documentation.Integrations/S3/S3IncrementalUploader.cs
src/services/Elastic.Documentation.Integrations/S3/S3IncrementalUploader.cs
Show resolved
Hide resolved
Contributor
Author
|
@reakaleek or @Mpdreamz, I don't have Snyk admin access so I need an approval there for the Integrations project. Ready for review despite Github's status! |
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
4 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.
This pull request introduces a new modular integration for S3 uploads and incremental changelog artifact deployment. The most significant changes are the extraction and generalization of S3 ETag calculation and upload logic into a new
Elastic.Documentation.Integrationsproject, and the addition of a newchangelog uploadcommand that enables incremental uploads of changelog artifacts to S3. Several services and tests are updated to use the new integration, improving code reusability and maintainability.New S3 integration module and incremental upload support:
Elastic.Documentation.Integrations, which provides reusable S3 integration components, includingS3EtagCalculatorfor ETag calculation andS3IncrementalUploaderfor efficient, content-based incremental uploads. [1] [2] [3]Changelog upload command and service:
changelog uploadsubcommand to the CLI, supporting incremental upload of changelog or bundle artifacts to S3 (and a stub for Elasticsearch). This uses the new S3 integration for efficient uploads. [1] [2] [3] [4]Codebase refactoring and dependency updates:
AwsS3SyncPlanStrategyand related assembler code, replacing it with calls to the new shared integration library. [1] [2] [3]These changes modularize S3 upload logic, enable incremental changelog artifact deployment, and improve code reuse across services.