-
Notifications
You must be signed in to change notification settings - Fork 123
[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 <444668+tobio@users.noreply.github.com>
Co-authored-by: tobio <444668+tobio@users.noreply.github.com>
Co-authored-by: tobio <444668+tobio@users.noreply.github.com>
Co-authored-by: tobio <444668+tobio@users.noreply.github.com>
Co-authored-by: tobio <444668+tobio@users.noreply.github.com>
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 <444668+tobio@users.noreply.github.com>
| provider "elasticstack" { | ||
| elasticsearch {} | ||
| } | ||
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
…tion Co-authored-by: tobio <444668+tobio@users.noreply.github.com>
|
@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 <444668+tobio@users.noreply.github.com>
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. |
Co-authored-by: tobio <444668+tobio@users.noreply.github.com>
All PR feedback has been addressed in commit 3c8a8b2:
|
…e validation Co-authored-by: tobio <444668+tobio@users.noreply.github.com>
252fca6 to
92ee168
Compare
…4f89-4206-9e30-02dc9febbc48 * origin/main: (45 commits) Add resource to manage ML datafeed state (#1422) Extract agent policy test config (#1439) chore(deps): update golang:1.25.4 docker digest to e68f6a0 (#1437) chore(deps): update kibana-openapi-spec digest to 6a867d8 (#1435) chore(deps): update docker.elastic.co/kibana/kibana docker tag to v9.2.1 (#1434) chore(deps): update docker.elastic.co/elasticsearch/elasticsearch docker tag to v9.2.1 (#1433) First take at documented coding standards (#1429) chore(deps): update golangci/golangci-lint-action action to v9 (#1431) chore(deps): update golang:1.25.4 docker digest to 6ca9eb0 (#1432) chore(deps): update kibana-openapi-spec digest to 96ffcd7 (#1430) chore(deps): update kibana-openapi-spec digest to a8e3b64 (#1428) chore(deps): update golang docker tag to v1.25.4 (#1423) Update the system_user acceptance tests to utilise the config directory pattern (#1404) Add an issue template for covering new stack features (#1418) chore(deps): update golang:1.25.3 docker digest to 6d4e5e7 (#1421) Update provider description (#1405) chore(deps): update golang:1.25.3 docker digest to 0afe9b5 (#1416) chore(deps): update module github.com/golangci/golangci-lint to v2.6.1 (#1417) Added space awareness to Agent Policies (#1390) Fix data view color field format params not computed in Terraform state (#1414) ...
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.
Pull Request Overview
This PR introduces a new Terraform resource for managing Elasticsearch aliases with atomic operations, addressing the limitation where alias updates previously required two separate operations that could lead to data loss. The implementation uses nested attributes following plugin framework best practices and provides comprehensive test coverage.
Key changes:
- Introduces atomic alias management to prevent data loss during write index transitions
- Implements proper nested attribute schema design instead of blocks
- Includes comprehensive test coverage for various alias scenarios
Reviewed Changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| provider/plugin_framework.go | Registers the new alias resource |
| internal/elasticsearch/index/alias/*.go | Complete alias resource implementation with CRUD operations |
| internal/clients/elasticsearch/index.go | Adds atomic alias update API functions |
| internal/elasticsearch/index/index/schema.go | Converts index alias from block to nested attribute |
| internal/elasticsearch/index/index/update.go | Replaces maps.Equal with deepEqual for slice support |
| internal/elasticsearch/transform/transform_test.go | Updates tests to use new alias attribute syntax |
| internal/elasticsearch/index/index/acc_test.go | Updates tests to use new alias attribute syntax |
| examples/resources/elasticstack_elasticsearch_index/resource.tf | Updates example to use new alias attribute syntax |
| docs/resources/*.md | Documentation for new resource and updated index resource |
| // deepEqual compares two maps for deep equality, handling slices and other types | ||
| // that are not comparable with maps.Equal | ||
| func deepEqual(a, b map[string]interface{}) bool { |
Copilot
AI
Nov 13, 2025
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.
The function is named deepEqual but doesn't actually perform deep equality checking - it uses reflect.DeepEqual internally. Consider either renaming to mapsEqual or compareMapsDeeply to better reflect that this is specifically for comparing maps, or document that it's a wrapper around reflect.DeepEqual with map-specific checks.
| for _, config := range plannedConfigs { | ||
| action := elasticsearch.AliasAction{ | ||
| Type: "add", | ||
| Index: config.Name, | ||
| Alias: aliasName, | ||
| IsWriteIndex: config.IsWriteIndex, | ||
| Filter: config.Filter, | ||
| IndexRouting: config.IndexRouting, | ||
| IsHidden: config.IsHidden, | ||
| Routing: config.Routing, | ||
| SearchRouting: config.SearchRouting, | ||
| } | ||
| actions = append(actions, action) | ||
| } |
Copilot
AI
Nov 13, 2025
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.
The update logic unconditionally adds all planned indices, even those that haven't changed. This could be optimized by only adding indices that were actually modified (comparing with currentIndexMap), which would reduce unnecessary API operations and improve performance for large alias configurations.
| if action.IsHidden { | ||
| addDetails["is_hidden"] = action.IsHidden | ||
| } |
Copilot
AI
Nov 13, 2025
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.
The condition if action.IsHidden will cause the field to be omitted when IsHidden is false, which could result in inconsistent behavior. If the API requires explicit false values to unset hidden status, this should be addDetails["is_hidden"] = action.IsHidden without the condition. If the API interprets absence as false, this should be documented with a comment.
| if action.IsHidden { | |
| addDetails["is_hidden"] = action.IsHidden | |
| } | |
| addDetails["is_hidden"] = action.IsHidden |
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.