Skip to content

Conversation

@freeznet
Copy link
Member

No description provided.

@freeznet freeznet self-assigned this Nov 18, 2025
@freeznet freeznet requested a review from a team as a code owner November 18, 2025 07:48
nlu90
nlu90 previously approved these changes Nov 26, 2025
Copy link

Copilot AI left a 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 adds support for SN Cloud secrets management by introducing a new Terraform resource and data source for handling secrets. The implementation allows users to create, read, update, and delete secrets in the StreamNative Cloud platform.

  • Adds streamnative_secret resource and data source for managing secrets
  • Moves k8s.io/api from indirect to direct dependency in go.mod
  • Includes a delete timeout configuration for Pulsar clusters (unrelated improvement)

Reviewed changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated 6 comments.

Show a summary per file
File Description
go.mod Promotes k8s.io/api to direct dependency for secret type support
examples/secrets/main.tf Provides example Terraform configuration for secret resource usage
docs/resources/secret.md Auto-generated documentation for streamnative_secret resource schema
docs/data-sources/secret.md Auto-generated documentation for streamnative_secret data source schema
cloud/secret_test.go Test suite covering secret creation, deletion, and external removal scenarios
cloud/resource_secret.go Implementation of CRUD operations for secret resource with support for data and string_data
cloud/data_source_secret.go Implementation of read operation for secret data source
cloud/provider.go Registers secret resource/data source and adds description entries
cloud/resource_pulsar_cluster.go Adds 30-minute delete timeout to prevent test failures (unrelated improvement)

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

You can also share your feedback on Copilot code review for a chance to win a $100 gift card. Take the survey.

Comment on lines +17 to +31
Importer: &schema.ResourceImporter{
StateContext: func(
ctx context.Context, d *schema.ResourceData, meta interface{}) ([]*schema.ResourceData, error) {
parts := strings.Split(d.Id(), "/")
if len(parts) != 2 {
return nil, fmt.Errorf("invalid import id %q, expected <organization>/<name>", d.Id())
}
_ = d.Set("organization", parts[0])
_ = d.Set("name", parts[1])
if diags := dataSourceSecretRead(ctx, d, meta); diags.HasError() {
return nil, fmt.Errorf("import %q: %s", d.Id(), diags[0].Summary)
}
return []*schema.ResourceData{d}, nil
},
},
Copy link

Copilot AI Nov 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Data sources should not have an Importer field. The Importer is only applicable to resources, not data sources. Data sources are read-only and don't maintain state that needs to be imported. Other data sources in the codebase (e.g., cloud/data_source_volume.go) do not define an Importer.

Suggested change
Importer: &schema.ResourceImporter{
StateContext: func(
ctx context.Context, d *schema.ResourceData, meta interface{}) ([]*schema.ResourceData, error) {
parts := strings.Split(d.Id(), "/")
if len(parts) != 2 {
return nil, fmt.Errorf("invalid import id %q, expected <organization>/<name>", d.Id())
}
_ = d.Set("organization", parts[0])
_ = d.Set("name", parts[1])
if diags := dataSourceSecretRead(ctx, d, meta); diags.HasError() {
return nil, fmt.Errorf("import %q: %s", d.Id(), diags[0].Summary)
}
return []*schema.ResourceData{d}, nil
},
},

Copilot uses AI. Check for mistakes.
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we have multiple existing data sources have Importer blocks, so we may remove such in future PRs, this PR will not cover such changes.

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines 87 to 116
"type": {
Type: schema.TypeString,
Optional: true,
Computed: true,
ForceNew: true,
Description: descriptions["secret_type"],
},
"data": {
Type: schema.TypeMap,
Optional: true,
Computed: true,
Sensitive: true,
ForceNew: true,
AtLeastOneOf: []string{"data", "string_data"},
Description: descriptions["secret_data"],
Elem: &schema.Schema{
Type: schema.TypeString,
},
},
"string_data": {
Type: schema.TypeMap,
Optional: true,
Sensitive: true,
ForceNew: true,
AtLeastOneOf: []string{"data", "string_data"},
Description: descriptions["secret_string_data"],
Elem: &schema.Schema{
Type: schema.TypeString,
},
},
Copy link

Copilot AI Nov 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All fields in the secret resource schema are marked as ForceNew: true, which means any change forces recreation of the resource. However, the resource defines an UpdateContext function that will never be called since there are no updatable fields.

Additionally, the test TestSecretUpdate expects to update fields like type, instance_name, data, and string_data, but these are all marked as ForceNew: true, which means Terraform will destroy and recreate the resource instead of updating it in-place.

Consider either:

  1. Removing the ForceNew: true constraint from fields that should be updatable (like type, data, string_data)
  2. Removing the UpdateContext handler and the update test if all fields truly require recreation

Copilot uses AI. Check for mistakes.
Comment on lines +70 to +73
Timeouts: &schema.ResourceTimeout{
// Pulsar clusters can take time to tear down; allow 30m to avoid spurious test failures.
Delete: schema.DefaultTimeout(30 * time.Minute),
},
Copy link

Copilot AI Nov 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nitpick] This timeout configuration change appears unrelated to the PR's stated purpose of adding SN Cloud secrets support. Consider moving this change to a separate PR or adding context to the PR description explaining why this is included.

Suggested change
Timeouts: &schema.ResourceTimeout{
// Pulsar clusters can take time to tear down; allow 30m to avoid spurious test failures.
Delete: schema.DefaultTimeout(30 * time.Minute),
},

Copilot uses AI. Check for mistakes.
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 11 out of 11 changed files in this pull request and generated 3 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +95 to +106
ForceNew: true,
AtLeastOneOf: []string{"data", "string_data"},
Description: descriptions["secret_data"],
Elem: &schema.Schema{
Type: schema.TypeString,
},
},
"string_data": {
Type: schema.TypeMap,
Optional: true,
Sensitive: true,
ForceNew: true,
Copy link

Copilot AI Dec 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The data and string_data fields both have ForceNew: true, which means changing either field should force resource recreation. However, the test TestSecretUpdate at lines 140-151 attempts to switch from data (initialData) to string_data (updatedStringData) without recreating the resource. This is contradictory to the ForceNew flag and may cause unexpected behavior.

Either:

  1. Remove ForceNew: true from both fields if updates should be allowed without recreation, or
  2. Update the test to expect resource recreation when these fields change.

Copilot uses AI. Check for mistakes.
page_title: "streamnative_secret Resource - terraform-provider-streamnative"
subcategory: ""
description: |-
Copy link

Copilot AI Dec 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The description field is empty. Consider adding a brief description of what this resource does, such as: "Manages StreamNative Cloud secrets for storing sensitive data like credentials and tokens."

Suggested change
Manages StreamNative Cloud secrets for storing sensitive data like credentials and tokens.

Copilot uses AI. Check for mistakes.
page_title: "streamnative_secret Data Source - terraform-provider-streamnative"
subcategory: ""
description: |-
Copy link

Copilot AI Dec 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The description field is empty. Consider adding a brief description of what this data source does, such as: "Retrieves information about an existing StreamNative Cloud secret."

Suggested change
Retrieves information about an existing StreamNative Cloud secret.

Copilot uses AI. Check for mistakes.
@freeznet freeznet merged commit 9c344ee into main Dec 8, 2025
7 of 8 checks passed
@freeznet freeznet deleted the freeznet/support-secrets branch December 8, 2025 12:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants