Conversation
There was a problem hiding this comment.
Pull request overview
This PR aims to fix maintenance window updates by ensuring the Pulsar cluster resource state is fully populated from the API during Read, and it extends test coverage to validate key identity attributes in state.
Changes:
- Populate
organization,name,instance_name,location, andpool_member_nameinresourcePulsarClusterReadvia a new helper. - Add additional acceptance test assertions for identity fields to detect state drift.
- Add unit tests for the new identity-state helper.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
cloud/resource_pulsar_cluster.go |
Adds setPulsarClusterIdentityState and calls it during Read to sync identity fields into Terraform state. |
cloud/pulsar_cluster_test.go |
Adds acceptance checks asserting identity attributes are present/consistent in state. |
cloud/pulsar_cluster_state_test.go |
New unit tests validating the identity-state helper for hosted and BYOC clusters. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if diagErr := setPulsarClusterIdentityState(d, pulsarCluster); diagErr != nil { | ||
| return diagErr | ||
| } | ||
| namespace = pulsarCluster.Namespace |
There was a problem hiding this comment.
Calling setPulsarClusterIdentityState() during every Read will populate name from the API even when the user omitted name (the resource supports auto-generated names via name being Optional). With an Optional-but-not-Computed attribute, this can introduce persistent diffs (state has generated name, config has empty) and can block applying unrelated updates (e.g., maintenance_window) because Update rejects d.HasChange("name"). Consider limiting this state population to import only (set org/name in Importer StateContext like other resources), or make name (and any other server-populated optional fields) Computed: true (Optionally with Optional: true) so Terraform doesn’t plan to unset it when not configured.
| func setPulsarClusterIdentityState(d *schema.ResourceData, pulsarCluster *cloudv1alpha1.PulsarCluster) diag.Diagnostics { | ||
| if err := d.Set("organization", pulsarCluster.Namespace); err != nil { | ||
| return diag.FromErr(fmt.Errorf("ERROR_SET_ORGANIZATION: %w", err)) | ||
| } | ||
| if err := d.Set("name", pulsarCluster.Name); err != nil { | ||
| return diag.FromErr(fmt.Errorf("ERROR_SET_NAME: %w", err)) | ||
| } | ||
| if err := d.Set("instance_name", pulsarCluster.Spec.InstanceName); err != nil { | ||
| return diag.FromErr(fmt.Errorf("ERROR_SET_INSTANCE_NAME: %w", err)) | ||
| } | ||
| if err := d.Set("location", pulsarCluster.Spec.Location); err != nil { | ||
| return diag.FromErr(fmt.Errorf("ERROR_SET_LOCATION: %w", err)) | ||
| } | ||
| if err := d.Set("pool_member_name", pulsarCluster.Spec.PoolMemberRef.Name); err != nil { | ||
| return diag.FromErr(fmt.Errorf("ERROR_SET_POOL_MEMBER_NAME: %w", err)) | ||
| } | ||
| return nil |
There was a problem hiding this comment.
setPulsarClusterIdentityState() sets name directly into state. Since name is Optional (not Computed) and can be omitted for auto-generated cluster names, writing the API-generated value into state can cause perpetual diffs/failed applies when configuration leaves name unset. If the intent is to fix imports, prefer setting these fields only during import, or update the schema to make name Optional+Computed (or otherwise suppress diffs) when server-generated.
| resource.TestCheckResourceAttr("streamnative_pulsar_cluster.test-pulsar-cluster", "organization", "sndev"), | ||
| resource.TestCheckResourceAttr("streamnative_pulsar_cluster.test-pulsar-cluster", "name", clusterGeneratedName), | ||
| resource.TestCheckResourceAttr("streamnative_pulsar_cluster.test-pulsar-cluster", "instance_name", clusterGeneratedName), | ||
| resource.TestCheckResourceAttr("streamnative_pulsar_cluster.test-pulsar-cluster", "location", "us-central1"), | ||
| resource.TestCheckResourceAttr("streamnative_pulsar_cluster.test-pulsar-cluster", "pool_member_name", ""), |
There was a problem hiding this comment.
Current acceptance checks only validate identity attributes when name is explicitly set in config. To guard against regressions for the documented auto-generated name flow (name omitted), add an acceptance test step that creates a cluster without name, then updates maintenance_window, and asserts a clean plan/apply (no forced updates/errors due to name drift).
|
fix in #150 |
No description provided.