-
Notifications
You must be signed in to change notification settings - Fork 409
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
[Internal] Migrate databricks_clusters
data source to plugin framework
#3998
Conversation
…ricks into migrate-cluster-plugin-framework
…ricks into migrate-cluster-plugin-framework
…ricks into migrate-cluster-plugin-framework
…ricks into migrate-cluster-plugin-framework
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.
Initial comments
} | ||
|
||
func (d *ClustersDataSource) Metadata(ctx context.Context, req datasource.MetadataRequest, resp *datasource.MetadataResponse) { | ||
resp.TypeName = "databricks_clusters_pluginframework" |
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.
Ditto here
if apierr.IsMissing(err) { | ||
resp.State.RemoveResource(ctx) | ||
} |
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.
Would this happen? Is it an error if there are no clusters? (Probably not.)
if name_contains != "" && !match_name { | ||
continue | ||
} | ||
ids = append(ids, types.StringValue(v.ClusterId)) |
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.
strings.Contains(any string, "") should always be true.
if name_contains != "" && !match_name { | |
continue | |
} | |
ids = append(ids, types.StringValue(v.ClusterId)) | |
if match_name { | |
ids = append(ids, types.StringValue(v.ClusterId)) | |
} |
} | ||
|
||
ids := make([]types.String, 0, len(clusters)) | ||
name_contains := strings.ToLower(clustersInfo.ClusterNameContains.ValueString()) |
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.
can we use camelCase?
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.
Maybe the work for setComputed could be in a separate pr?
type ClusterInfo struct { | ||
ClusterId types.String `tfsdk:"cluster_id" tf:"optional,computed"` | ||
Name types.String `tfsdk:"cluster_name" tf:"optional,computed"` | ||
ClusterInfo *compute_tf.ClusterDetails `tfsdk:"cluster_info" tf:"optional,computed"` |
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.
I think we should also try adding a proto level annotation for making a field computed in the tf configuration, so the generated tfsdk structs will have these
WIP
databricks_cluster
data source to plugin framework #3988Changes
Migrate
databricks_clusters
data source to plugin frameworkTests
Integration tests are passing.
make test
run locallydocs/
folderinternal/acceptance