-
Notifications
You must be signed in to change notification settings - Fork 8
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
Add support for network polices #669
Conversation
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.
Overall lgtm!
pkg/materialize/network_policy.go
Outdated
Rules []NetworkPolicyRule | ||
} | ||
|
||
type networkPolicyQueryResult struct { |
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.
Noticed the casing here is different from the standard. Is that on purpose
return diag.FromErr(err) | ||
} | ||
|
||
SetId(string(region), "network_policies", "", "", d) |
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.
nit: Noticed we use d.SetId in some places and SetId in other places. Any reason why we don't use d.SetId(transformIdWithRegion...
here?
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.
Ah yes, I've created that SetId
helper function for all data sources that accept database or schema names. But in this case this is not needed so better to use d.SetId directly!
terraform-provider-materialize/pkg/datasources/utils.go
Lines 10 to 21 in 4f61ea1
func SetId(region, resource, databaseName, schemaName string, d *schema.ResourceData) { | |
var id string | |
if databaseName != "" && schemaName != "" { | |
id = fmt.Sprintf("%s|%s|%s", databaseName, schemaName, resource) | |
} else if databaseName != "" { | |
id = fmt.Sprintf("%s|%s", databaseName, resource) | |
} else { | |
id = resource | |
} | |
d.SetId(utils.TransformIdWithRegion(region, id)) | |
} |
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.
High quality as always, thanks for getting this one across the line Bobby!
### Optional | ||
|
||
- `comment` (String) **Public Preview** Comment on an object in the database. | ||
- `region` (String) The region to use for the resource connection. If not set, the default region is used. |
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'm unfamiliar with the Terraform provider, is region
a field we typically allow folks to specify?
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.
Yep, this was introduced last year when we refactored the provider, this allows users to manage cross region instances within the same terraform project.
For example:
resource "materialize_database" "database" {
name = "example_database"
comment = "database comment"
}
# Create in separate region
resource "materialize_database" "database_us_west" {
name = "example_database"
comment = "database comment"
region = "aws/us-west-2"
}
the first one will create the db in the default region defined in the provider configuration, but the second one overrides the region and will create the db in aws/us-west-2
if it is enabled.
@@ -1,6 +1,7 @@ | |||
module github.com/MaterializeInc/terraform-provider-materialize | |||
|
|||
go 1.22.0 | |||
go 1.22.7 |
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.
just double checking, is this kind of upgrade a breaking change? /compatible with how we're changing the version of the terraform provider?
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.
Yep, this minor version upgrade should be ok, it was needed because of a recent dependabot commit:
Kind of sneaking the bump into this PR 😅
Rules []byte `db:"rules"` | ||
} | ||
|
||
var networkPolicyQuery = NewBaseQuery(` |
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.
is it worth making this a builtin view?
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.
Actually, this will be quite helpful!
// if err := d.Set("ownership_role", policy.OwnerName.String); err != nil { | ||
// return diag.FromErr(err) | ||
// } |
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.
Did you mean to leave this in?
// ownership not currently supported | ||
// if v, ok := d.GetOk("ownership_role"); ok { | ||
// ownership := materialize.NewOwnershipBuilder(metaDb, o) | ||
// if err := ownership.Alter(v.(string)); err != nil { | ||
// log.Printf("[DEBUG] resource failed ownership, dropping object: %s", o.Name) | ||
// b.Drop() | ||
// return diag.FromErr(err) | ||
// } | ||
// } |
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.
similar to above, did you mean to leave this in?
Thank you both @SangJunBak @ParkMyCar for the review! Very much appreciated 🙇 |
Fixes #666 😈
Adding a new
materialize_network_policy
resource:Also adding a data source to allow users to retrieve all network policies:
This also adds support for the new
CREATENETWORKPOLICY
system privilege, eg: