You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "dttung2905 (via GitHub)" <gi...@apache.org> on 2026/03/31 16:21:31 UTC
[PR] Add `iceberg_polaris_principal_role` resource [iceberg-terraform]
dttung2905 opened a new pull request, #31:
URL: https://github.com/apache/iceberg-terraform/pull/31
Hi team,
I want to propose the following changes in the PR
- `iceberg_polaris_principal_role` resource based on the open api spec [here](https://raw.githubusercontent.com/apache/polaris/refs/heads/main/spec/polaris-management-service.yml) for polaris
- Remove `catalog_name` field in the provider block. It was an oversight from me to include this in the previous PR as `catalog_name` should not be at the provider scope. Ultimately, we would like to keep a single provider block for all catalog names in a polaris deployment
- Minor changes with golangci linter for some of the existing files
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Add `iceberg_polaris_principal_role` resource [iceberg-terraform]
Posted by "dttung2905 (via GitHub)" <gi...@apache.org>.
dttung2905 commented on code in PR #31:
URL: https://github.com/apache/iceberg-terraform/pull/31#discussion_r3178647541
##########
internal/provider/resource_polaris_principal_test.go:
##########
@@ -36,6 +36,7 @@ func testAccPolarisProviderConfigWithToken(catalogURI, managementURI, token stri
if token != "" {
tokenAttr = fmt.Sprintf("\n token = %q", token)
}
+
Review Comment:
Ahh. I will remove this. It comes from the golangci linter iinw
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Add `iceberg_polaris_principal_role` resource [iceberg-terraform]
Posted by "rambleraptor (via GitHub)" <gi...@apache.org>.
rambleraptor commented on code in PR #31:
URL: https://github.com/apache/iceberg-terraform/pull/31#discussion_r3157700363
##########
internal/provider/resource_polaris_principal_role.go:
##########
@@ -0,0 +1,350 @@
+// Licensed to the Apache Software Foundation (ASF) under one or more
+// contributor license agreements. See the NOTICE file distributed with
+// this work for additional information regarding copyright ownership.
+// The ASF licenses this file to You under the Apache License, Version 2.0
+// (the "License"); you may not use this file except in compliance with
+// the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package provider
+
+import (
+ "context"
+ "errors"
+
+ "github.com/hashicorp/terraform-plugin-framework/diag"
+ "github.com/hashicorp/terraform-plugin-framework/path"
+ "github.com/hashicorp/terraform-plugin-framework/resource"
+ "github.com/hashicorp/terraform-plugin-framework/resource/schema"
+ "github.com/hashicorp/terraform-plugin-framework/resource/schema/booldefault"
+ "github.com/hashicorp/terraform-plugin-framework/resource/schema/boolplanmodifier"
+ "github.com/hashicorp/terraform-plugin-framework/resource/schema/planmodifier"
+ "github.com/hashicorp/terraform-plugin-framework/resource/schema/stringplanmodifier"
+ "github.com/hashicorp/terraform-plugin-framework/types"
+ "github.com/hashicorp/terraform-plugin-log/tflog"
+)
+
+var (
+ _ resource.Resource = &polarisPrincipalRoleResource{}
+ _ resource.ResourceWithImportState = &polarisPrincipalRoleResource{}
+)
+
+func NewPolarisPrincipalRoleResource() resource.Resource {
+ return &polarisPrincipalRoleResource{}
+}
+
+type polarisPrincipalRoleResource struct {
+ provider *icebergProvider
+ managementClient *polarisManagementClient
+}
+
+type polarisPrincipalRoleResourceModel struct {
+ ID types.String `tfsdk:"id"`
+ Name types.String `tfsdk:"name"`
+ Federated types.Bool `tfsdk:"federated"`
+ Properties types.Map `tfsdk:"properties"`
+}
+
+func (r *polarisPrincipalRoleResource) Metadata(_ context.Context, req resource.MetadataRequest, resp *resource.MetadataResponse) {
+ resp.TypeName = req.ProviderTypeName + "_polaris_principal_role"
+}
+
+func (r *polarisPrincipalRoleResource) Schema(_ context.Context, _ resource.SchemaRequest, resp *resource.SchemaResponse) {
+ resp.Schema = schema.Schema{
+ Description: "Manages a Polaris principal role (management API).",
+ Attributes: map[string]schema.Attribute{
+ "id": schema.StringAttribute{
Review Comment:
Where does this id value come from?
##########
internal/provider/provider.go:
##########
@@ -44,13 +42,11 @@ func New() func() provider.Provider {
// polarisSettingsModel is the Terraform-facing shape of the nested polaris_settings block.
type polarisSettingsModel struct {
ManagementURI types.String `tfsdk:"management_uri"`
- CatalogName types.String `tfsdk:"catalog_name"`
}
// polarisConfig holds Polaris-specific runtime configuration derived from polaris_settings.
type polarisConfig struct {
managementURI string
- catalogName string
Review Comment:
I'm not a Polaris expert. Why don't we need catalogName anymore?
##########
internal/provider/resource_polaris_principal_test.go:
##########
@@ -36,6 +36,7 @@ func testAccPolarisProviderConfigWithToken(catalogURI, managementURI, token stri
if token != "" {
tokenAttr = fmt.Sprintf("\n token = %q", token)
}
+
Review Comment:
Can we get rid of these stray lines? It clutters up the review.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Add `iceberg_polaris_principal_role` resource [terraform-provider-iceberg]
Posted by "dttung2905 (via GitHub)" <gi...@apache.org>.
dttung2905 closed pull request #31: Add `iceberg_polaris_principal_role` resource
URL: https://github.com/apache/terraform-provider-iceberg/pull/31
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Add `iceberg_polaris_principal_role` resource [iceberg-terraform]
Posted by "dttung2905 (via GitHub)" <gi...@apache.org>.
dttung2905 commented on PR #31:
URL: https://github.com/apache/iceberg-terraform/pull/31#issuecomment-4163986099
Hi @zeroshade, when you have time, could you also help to review this ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Add `iceberg_polaris_principal_role` resource [iceberg-terraform]
Posted by "dttung2905 (via GitHub)" <gi...@apache.org>.
dttung2905 commented on code in PR #31:
URL: https://github.com/apache/iceberg-terraform/pull/31#discussion_r3178646241
##########
internal/provider/provider.go:
##########
@@ -44,13 +42,11 @@ func New() func() provider.Provider {
// polarisSettingsModel is the Terraform-facing shape of the nested polaris_settings block.
type polarisSettingsModel struct {
ManagementURI types.String `tfsdk:"management_uri"`
- CatalogName types.String `tfsdk:"catalog_name"`
}
// polarisConfig holds Polaris-specific runtime configuration derived from polaris_settings.
type polarisConfig struct {
managementURI string
- catalogName string
Review Comment:
I dropped catalogName from polarisConfig because nothing in this provider reads it yet. Principal / principal-role management APIs (/principals, /principal-roles) are defined without a catalog segment in path; Polaris implements them without a catalog parameter.
Paths that do carry catalogName (e.g. /catalogs/{catalogName}/catalog-roles/...) should expose catalog_name on the specific resource or data source that calls those endpoints, not as a generic default on the provider. That avoids assuming a single catalog when a config might talk to multiple catalogs over time.
https://editor.swagger.io/?url=https://raw.githubusercontent.com/apache/polaris/refs/heads/main/spec/polaris-management-service.yml
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Add `iceberg_polaris_principal_role` resource [iceberg-terraform]
Posted by "dttung2905 (via GitHub)" <gi...@apache.org>.
dttung2905 commented on PR #31:
URL: https://github.com/apache/iceberg-terraform/pull/31#issuecomment-4165278436
Hi @flyrain, since you helped review my previous pr, could you please also help with this one? Thanks alot in advance
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Add `iceberg_polaris_principal_role` resource [iceberg-terraform]
Posted by "dttung2905 (via GitHub)" <gi...@apache.org>.
dttung2905 commented on code in PR #31:
URL: https://github.com/apache/iceberg-terraform/pull/31#discussion_r3178646992
##########
internal/provider/resource_polaris_principal_role.go:
##########
@@ -0,0 +1,350 @@
+// Licensed to the Apache Software Foundation (ASF) under one or more
+// contributor license agreements. See the NOTICE file distributed with
+// this work for additional information regarding copyright ownership.
+// The ASF licenses this file to You under the Apache License, Version 2.0
+// (the "License"); you may not use this file except in compliance with
+// the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package provider
+
+import (
+ "context"
+ "errors"
+
+ "github.com/hashicorp/terraform-plugin-framework/diag"
+ "github.com/hashicorp/terraform-plugin-framework/path"
+ "github.com/hashicorp/terraform-plugin-framework/resource"
+ "github.com/hashicorp/terraform-plugin-framework/resource/schema"
+ "github.com/hashicorp/terraform-plugin-framework/resource/schema/booldefault"
+ "github.com/hashicorp/terraform-plugin-framework/resource/schema/boolplanmodifier"
+ "github.com/hashicorp/terraform-plugin-framework/resource/schema/planmodifier"
+ "github.com/hashicorp/terraform-plugin-framework/resource/schema/stringplanmodifier"
+ "github.com/hashicorp/terraform-plugin-framework/types"
+ "github.com/hashicorp/terraform-plugin-log/tflog"
+)
+
+var (
+ _ resource.Resource = &polarisPrincipalRoleResource{}
+ _ resource.ResourceWithImportState = &polarisPrincipalRoleResource{}
+)
+
+func NewPolarisPrincipalRoleResource() resource.Resource {
+ return &polarisPrincipalRoleResource{}
+}
+
+type polarisPrincipalRoleResource struct {
+ provider *icebergProvider
+ managementClient *polarisManagementClient
+}
+
+type polarisPrincipalRoleResourceModel struct {
+ ID types.String `tfsdk:"id"`
+ Name types.String `tfsdk:"name"`
+ Federated types.Bool `tfsdk:"federated"`
+ Properties types.Map `tfsdk:"properties"`
+}
+
+func (r *polarisPrincipalRoleResource) Metadata(_ context.Context, req resource.MetadataRequest, resp *resource.MetadataResponse) {
+ resp.TypeName = req.ProviderTypeName + "_polaris_principal_role"
+}
+
+func (r *polarisPrincipalRoleResource) Schema(_ context.Context, _ resource.SchemaRequest, resp *resource.SchemaResponse) {
+ resp.Schema = schema.Schema{
+ Description: "Manages a Polaris principal role (management API).",
+ Attributes: map[string]schema.Attribute{
+ "id": schema.StringAttribute{
Review Comment:
Terraform requires a stable resource identifier separate from lifecycle arguments. Here id is computed and set to the same value as name for this resource: Polaris identifies a principal role by name, so after CreatePrincipalRole succeeds we persist name as id.
That matches iceberg_polaris_principal and keeps terraform import simple (import ID = principal role name). The schema uses UseStateForUnknown on id so plans stay quiet after first apply.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org