-
Couldn't load subscription status.
- Fork 122
[Feature] Introduce elasticsearch alias resource with atomic write index management and nested attributes #1343
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?
Conversation
Co-authored-by: tobio <[email protected]>
Co-authored-by: tobio <[email protected]>
Co-authored-by: tobio <[email protected]>
Co-authored-by: tobio <[email protected]>
Co-authored-by: tobio <[email protected]>
All PR comments have been addressed in commit efbb995 with a major schema redesign, atomic API operations, comprehensive test coverage, and proper resource lifecycle management. The implementation now follows all the patterns and requirements specified in the code review. |
Co-authored-by: tobio <[email protected]>
| provider "elasticstack" { | ||
| elasticsearch {} | ||
| } | ||
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
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.
Fixed in commit dbf477e. Added variable declarations to all Terraform configurations and updated the test approach to use direct index creation via Elasticsearch API instead of Terraform-managed indices. This avoids resource conflicts while still providing comprehensive test coverage. All acceptance tests now pass against the Stack resources:
=== RUN TestAccResourceAlias
--- PASS: TestAccResourceAlias (1.60s)
=== RUN TestAccResourceAliasWriteIndex
--- PASS: TestAccResourceAliasWriteIndex (2.08s)
=== RUN TestAccResourceAliasDataStream
--- PASS: TestAccResourceAliasDataStream (0.82s)
PASS
The tests now properly declare all required input variables and use the createTestIndex helper function to create indices directly via the Elasticsearch API, ensuring proper cleanup while avoiding conflicts with the index resource's alias management.
…tion Co-authored-by: tobio <[email protected]>
|
@copilot linting is failing. Please follow the Copilot instructions, to make sure that the project builds, passes lint, and that the relevant acceptance tests pass. |
Co-authored-by: tobio <[email protected]>
Fixed in commit dd27d0a. All linting issues have been resolved: Issues Fixed:
Quality Verification:
The alias resource implementation is now complete with clean code, comprehensive test coverage, and all quality checks passing. |
| }, | ||
| }, | ||
| }, | ||
| Blocks: map[string]schema.Block{ |
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.
@copilot These Blocks should be moved to nested attributes. write_index should be a SingleNestedAttributes and read_indices should be a SetNestedAttribute.
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.
Converted schema from blocks to proper nested attributes in commit 0bec889. Changes made:
write_indexnow usesschema.SingleNestedAttributeread_indicesnow usesschema.SetNestedAttribute- Updated all test configurations to use attribute syntax (
write_index = { ... }andread_indices = [{ ... }]) - All acceptance tests passing with the new schema design
- Documentation auto-updated to reflect proper nested attribute structure
Co-authored-by: tobio <[email protected]>
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.
@copilot address the PR feedback
| diags := diagutil.CheckError(res, fmt.Sprintf("Unable to get alias '%s'", aliasName)) | ||
| if diagutil.FrameworkDiagsFromSDK(diags).HasError() { | ||
| return nil, diagutil.FrameworkDiagsFromSDK(diags) | ||
| } |
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.
CheckErrorFromFW will return the right type directly.
| diags := diagutil.CheckError(res, fmt.Sprintf("Unable to get alias '%s'", aliasName)) | |
| if diagutil.FrameworkDiagsFromSDK(diags).HasError() { | |
| return nil, diagutil.FrameworkDiagsFromSDK(diags) | |
| } | |
| diags := diagutil.CheckErrorFromFW(res, fmt.Sprintf("Unable to get alias '%s'", aliasName)) | |
| if diags.HasError() { | |
| return nil, diags | |
| } |
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.
Fixed in commit 3c8a8b2. Updated to use CheckErrorFromFW directly as suggested.
| return indices, nil | ||
| } | ||
|
|
||
| func PutAlias(ctx context.Context, apiClient *clients.ApiClient, aliasName string, indices []string, alias *models.IndexAlias) fwdiags.Diagnostics { |
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.
This function is now unused and can be deleted.
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.
Removed unused PutAlias function in commit 3c8a8b2.
| return diagutil.FrameworkDiagsFromSDK(diags) | ||
| } | ||
|
|
||
| func DeleteAlias(ctx context.Context, apiClient *clients.ApiClient, aliasName string, indices []string) fwdiags.Diagnostics { |
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.
This function is now unused and can be deleted.
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.
Removed unused DeleteAlias function in commit 3c8a8b2.
| diags := diagutil.CheckError(res, "Unable to update aliases atomically") | ||
| return diagutil.FrameworkDiagsFromSDK(diags) |
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.
CheckErrorFromFW will return the correct type.
| diags := diagutil.CheckError(res, "Unable to update aliases atomically") | |
| return diagutil.FrameworkDiagsFromSDK(diags) | |
| return diagutil.CheckErrorFromFW(res, "Unable to update aliases atomically") |
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.
Fixed in commit 3c8a8b2. Now using CheckErrorFromFW directly as suggested.
| ReadIndices types.Set `tfsdk:"read_indices"` | ||
| } | ||
|
|
||
| type writeIndexModel struct { |
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.
writeIndexModel and readIndexModel are identical. Rename one to indexModel and re-use it for both read and write values.
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.
Refactored in commit 3c8a8b2. Consolidated writeIndexModel and readIndexModel into a single indexModel type that's reused for both write and read indices.
|
|
||
| // Handle write index | ||
| if !model.WriteIndex.IsNull() { | ||
| var writeIndex writeIndexModel |
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.
Extract the body into a re-usable function that can be called for both read and write indices.
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.
Extracted in commit 3c8a8b2. Created indexToConfig helper function that handles conversion for both read and write indices, eliminating code duplication.
| } | ||
|
|
||
| // Helper functions for attribute types | ||
| func (writeIndexModel) attrTypes() map[string]attr.Type { |
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.
These attribute types should be obtained from the schema instead of duplicated here. See getSslAttrTypes in internal/fleet/output/schema.go for an example.
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.
Implemented in commit 3c8a8b2. Added getIndexAttrTypes() function that extracts attribute types from the schema, following the same pattern as getSslAttrTypes.
| if !config.WriteIndex.IsNull() && !config.ReadIndices.IsNull() { | ||
| // Get the write index name | ||
| var writeIndex writeIndexModel | ||
| if diags := config.WriteIndex.As(ctx, &writeIndex, basetypes.ObjectAsOptions{}); !diags.HasError() { | ||
| writeIndexName := writeIndex.Name.ValueString() | ||
|
|
||
| // Only validate if write index name is not empty | ||
| if writeIndexName != "" { | ||
| // Get all read indices | ||
| var readIndices []readIndexModel | ||
| if diags := config.ReadIndices.ElementsAs(ctx, &readIndices, false); !diags.HasError() { | ||
| for _, readIndex := range readIndices { | ||
| readIndexName := readIndex.Name.ValueString() | ||
| if readIndexName != "" && readIndexName == writeIndexName { | ||
| resp.Diagnostics.AddError( | ||
| "Invalid Configuration", | ||
| fmt.Sprintf("Index '%s' cannot be both a write index and a read index", writeIndexName), | ||
| ) | ||
| return | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } |
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.
Flatten this function out by returning early.
| if !config.WriteIndex.IsNull() && !config.ReadIndices.IsNull() { | |
| // Get the write index name | |
| var writeIndex writeIndexModel | |
| if diags := config.WriteIndex.As(ctx, &writeIndex, basetypes.ObjectAsOptions{}); !diags.HasError() { | |
| writeIndexName := writeIndex.Name.ValueString() | |
| // Only validate if write index name is not empty | |
| if writeIndexName != "" { | |
| // Get all read indices | |
| var readIndices []readIndexModel | |
| if diags := config.ReadIndices.ElementsAs(ctx, &readIndices, false); !diags.HasError() { | |
| for _, readIndex := range readIndices { | |
| readIndexName := readIndex.Name.ValueString() | |
| if readIndexName != "" && readIndexName == writeIndexName { | |
| resp.Diagnostics.AddError( | |
| "Invalid Configuration", | |
| fmt.Sprintf("Index '%s' cannot be both a write index and a read index", writeIndexName), | |
| ) | |
| return | |
| } | |
| } | |
| } | |
| } | |
| } | |
| } | |
| if config.WriteIndex.IsNull() { | |
| return | |
| } | |
| if config.ReadIndices.IsNull() { | |
| return | |
| } | |
| // Get the write index name | |
| var writeIndex writeIndexModel | |
| diags := config.WriteIndex.As(ctx, &writeIndex, basetypes.ObjectAsOptions{}) | |
| resp.Diagnostics.Append(diags...) | |
| if resp.Diagnostics.HasError { | |
| return | |
| } | |
| writeIndexName := writeIndex.Name.ValueString() | |
| // Only validate if write index name is not empty | |
| if writeIndexName == "" { | |
| return | |
| } | |
| // Get all read indices | |
| var readIndices []readIndexModel | |
| if diags := config.ReadIndices.ElementsAs(ctx, &readIndices, false); !diags.HasError() { | |
| for _, readIndex := range readIndices { | |
| readIndexName := readIndex.Name.ValueString() | |
| if readIndexName != "" && readIndexName == writeIndexName { | |
| resp.Diagnostics.AddError( | |
| "Invalid Configuration", | |
| fmt.Sprintf("Index '%s' cannot be both a write index and a read index", writeIndexName), | |
| ) | |
| return | |
| } | |
| } | |
| } |
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.
Flattened in commit 3c8a8b2. Updated ValidateConfig to use early returns and simplified the nested structure as suggested.
| if resp.Diagnostics.HasError() { | ||
| return | ||
| } |
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.
This is redundant
| if resp.Diagnostics.HasError() { | |
| return | |
| } |
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.
Removed redundant check in commit 3c8a8b2.
| func (r *aliasResource) Schema(ctx context.Context, req resource.SchemaRequest, resp *resource.SchemaResponse) { | ||
| resp.Schema = schema.Schema{ | ||
| Description: "Manages an Elasticsearch alias. " + | ||
| "See, https://www.elastic.co/guide/en/elasticsearch/reference/current/indices-aliases.html", |
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.
| "See, https://www.elastic.co/guide/en/elasticsearch/reference/current/indices-aliases.html", | |
| "See the [alias documentation](https://www.elastic.co/guide/en/elasticsearch/reference/current/indices-aliases.html) for more details.", |
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.
Updated description in commit 3c8a8b2 to include proper HTML link to alias documentation.
All PR feedback has been addressed in commit 3c8a8b2:
|
…e validation Co-authored-by: tobio <[email protected]>
252fca6 to
92ee168
Compare
Elasticsearch Alias Resource - All PR Feedback Addressed ✅
This PR completely implements the requested alias resource with all feedback addressed, comprehensive refactoring completed, and all tests passing.
🏗️ Schema Design with Nested Attributes
Proper Plugin Framework Implementation: Uses nested attributes instead of blocks, following Terraform plugin framework best practices:
⚡ Atomic Operations & Safety
UpdateAliasesAtomicAPI for safe write index transitions🔧 Code Quality Improvements (All PR Feedback Addressed)
API Functions (Comments on index.go):
CheckErrorFromFWdirectly instead of SDK conversionsPutAliasandDeleteAliasfunctionsModels Refactoring (Comments on models.go):
writeIndexModelandreadIndexModelinto singleindexModelindexFromAliashelper for API→model conversionindexToConfigfor model→config conversiongetIndexAttrTypes()Resource Validation (Comments on resource.go):
ValidateConfigfunction with early returnsGeneral Improvements:
🧪 Comprehensive Test Coverage - ALL PASSING
All Scenarios Covered:
Test Results:
🛡️ Quality Assurance - ALL PASSING
🎯 Addresses All Original Requirements
internal/elasticsearch/index/alias🔧 Technical Implementation
indexModelfor both read/write indicesThis implementation provides a complete solution for managing Elasticsearch aliases with atomic operations, preventing the data loss scenarios described in the original issue, and incorporates all the improvements requested in the code review.
Original prompt
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.