Conversation
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Pull Request Overview
Adds optional display name field and validation information to the BYOK (Bring Your Own Key) resource and data source, enabling users to set a human-readable alias for keys that can be updated after creation.
- Added optional
display_namefield that can be updated for existing BYOK keys - Added computed
validationfields (phase,since,message,region) to provide key validation status - Added update functionality to support changing the display name
Reviewed Changes
Copilot reviewed 13 out of 14 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| internal/provider/resource_byok_key.go | Added display_name field, validation schema, and update functionality |
| internal/provider/data_source_byok_key.go | Added display_name and validation fields to data source schema |
| internal/testdata/byok/*.json | Added test data files with display_name and validation fields |
| internal/provider/resource_byok_key_*_test.go | Enhanced tests to verify display_name updates and validation field handling |
| go.mod | Updated Go version and byok SDK dependency |
| .go-version | Updated Go version specification |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Steven Gagniere (sgagniere)
left a comment
Steven Gagniere (sgagniere)
left a comment
There was a problem hiding this comment.
Thanks!
Here's some comments
Analysis Details0 Issues
Coverage and Duplications
Project ID: terraform-provider-confluent |
Steven Gagniere (sgagniere)
left a comment
Steven Gagniere (sgagniere)
left a comment
There was a problem hiding this comment.
Hi, some more comments.
- From the test results, I see
~ validation_phase = "INITIALIZING" -> "VALID". We might want to add a "wait for sync/provision" function that will poll until the backend returns a desirable phase (in this case "VALID"). Is there any external action the user has to take to go from initialization -> valid? If not, we should consider adding the wait function. - Let's update the data source test files too. Since the data source tests use the same json files as the resource tests, I think all we need is a few extra test lines to check the validation fields.
Implement display name (alias) functionality for BYOK keys with in-place updates using the new BYOK SDK v0.0.8 PATCH capabilities. Core Changes: - Add optional display_name field to confluent_byok_key resource schema - Implement byokUpdate function using UpdateByokV1Key PATCH API - Update byokCreate to handle display_name during resource creation - Enhance setKeyAttributes to read/set display_name from API responses Testing Updates: - Add update test scenarios for AWS, Azure, and GCP - Fix WireMock scenario state management for create→update→verify flows - Add display_name validation in all acceptance tests
This commit implements support for the new validation object returned by the BYOK API v0.0.8, which provides key validation status information to help users monitor and troubleshoot their encryption keys. **Core Changes:** - Add validation schema with phase, since, message, and region fields - Update setKeyAttributes() to parse validation object from API responses - Add validation support to both resource and data source schemas - All validation fields are computed-only (no user configuration) - Update all testdata JSON files with realistic validation examples
airlock-confluentinc
bot
force-pushed
the
SP-991
branch
from
6b34a80 to
88d2836
Compare
| paramDisplayName: { | ||
| Type: schema.TypeString, | ||
| Description: "A human-readable name for the BYOK key.", | ||
| Optional: true, |
There was a problem hiding this comment.
Do we want to have both computed and optional here? What's the default value if user doesn't provide display_name in the request data?
There was a problem hiding this comment.
The backend does not compute a default value for this. It's fully optional.
Kostya Linou (linouk23)
left a comment
Kostya Linou (linouk23)
left a comment
There was a problem hiding this comment.
Looks great! Added quick question.
| createByokKeyRequest := byok.NewByokV1Key() | ||
|
|
||
| // Set display name | ||
| displayName := d.Get(paramDisplayName).(string) |
There was a problem hiding this comment.
I recall that some resources only assign this attribute if it is not empty. Will the server still process the request if name is an empty string ("")?
There was a problem hiding this comment.
Yes, an patching with empty works. Tried an example with the provider to be sure:
Changes to Outputs:
~ byok_key_display_name = "My AWS BYOK Encryption Key" -> ""
Apply complete! Resources: 0 added, 2 changed, 0 destroyed.
Outputs:
...
byok_key_display_name = ""
Release Notes
New Features
Phase,Since,MessageandRegionChecklist
Test & Reviewsection below.Blast Radiussection below.What
Blast Radius
confluent_byok_keyresource/data-source will be blocked.References
Test & Review
Generating a BYOK object utilizing new alias field