Conversation
|
Hi there, I'm the Modular magician. I've detected the following information about your changes: Diff reportYour PR generated some diffs in downstreams - here they are.
Missing test reportYour PR includes resource fields which are not covered by any test. Resource: resource "google_biglake_iceberg_table" "primary" {
location = # value needed
partition_spec {
fields {
name = # value needed
source_id = # value needed
transform = # value needed
}
}
schema {
fields {
doc = # value needed
}
identifier_field_ids = # value needed
}
}
|
|
Hi there, I'm the Modular magician. I've detected the following information about your changes: Diff reportYour PR generated some diffs in downstreams - here they are.
|
Tests analyticsTotal tests: 6087 Click here to see the affected service packages
Action takenFound 7 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests
|
|
🟢 Tests passed during RECORDING mode: 🟢 No issues found for passed tests after REPLAYING rerun. 🔴 Tests failed during RECORDING mode: 🔴 Errors occurred during RECORDING mode. Please fix them to complete your PR. |
|
Googlers: For automatic test runs see go/terraform-auto-test-runs. @c2thorn, a repository maintainer, has been assigned to review your changes. If you have not received review feedback within 2 business days, please leave a comment on this PR asking them to take a look. You can help make sure that review is quick by doing a self-review and by running impacted tests locally. |
There was a problem hiding this comment.
Hey @rambleraptor! hope you are doing well.
Added some initial comments.
This resource has a lot of custom code! Can you give some more comments or explanation as to what makes the resource so abnormal to need them? Is this just common for biglakeiceberg?
| if part != google.Underscore(apiPart) { | ||
| return false | ||
| } | ||
| if strings.Contains(apiPart, "-") { |
There was a problem hiding this comment.
@melinath can you take a quick peek at this change here
|
@c2thorn I should've put my usual disclaimer on this PR! The BigLake Iceberg resources have to conform to the Apache Iceberg REST Catalog Spec. This isn't a Google API standard at all. Our team has no control over this spec, but we have to follow it exactly or else we break users. This ultimately means that there's a ton of custom code involved since MMv1 has a lot of Google assumptions baked in. It's really bit us on the last few Iceberg resources... (background: The REST Catalog spec exists so that OSS engines like Apache Spark can talk to a variety of REST Catalogs - BigQuery, Databricks, Snowflake, other major companies - through a standard interface) |
|
@c2thorn changes made! |
|
Hi there, I'm the Modular magician. I've detected the following information about your changes: Diff reportYour PR generated some diffs in downstreams - here they are.
|
Non-exercised tests🔴 Tests were added that are skipped in VCR:
Tests analyticsTotal tests: 6113 Click here to see the affected service packages
Action takenFound 10 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests
|
|
🟢 Tests passed during RECORDING mode: 🟢 No issues found for passed tests after REPLAYING rerun. 🔴 Tests failed during RECORDING mode: 🔴 Errors occurred during RECORDING mode. Please fix them to complete your PR. |
|
@c2thorn This PR has been waiting for review for 3 weekdays. Please take a look! Use the label |
|
/gcbrun |
|
Hi there, I'm the Modular magician. I've detected the following information about your changes: Diff reportYour PR generated some diffs in downstreams - here they are.
|
|
I think credentials got rotated during the build :[ |
|
/gcbrun |
|
Hi there, I'm the Modular magician. I've detected the following information about your changes: Diff reportYour PR generated some diffs in downstreams - here they are.
|
Non-exercised tests🔴 Tests were added that are skipped in VCR:
Tests analyticsTotal tests: 6140 Click here to see the affected service packages
Action takenFound 1 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests
|
c2thorn
left a comment
There was a problem hiding this comment.
LGTM after reviewing how existing iceberg implementations.
| @@ -0,0 +1,12 @@ | |||
| var icebergTableIgnoredProperties = map[string]bool{ | |||
There was a problem hiding this comment.
If I were reviewing the other IcebergNamespace resource initially, maybe I would have pushed for separating out user-defined properties and server-defined properties into two separate fields. It's fine to suppress it this way in SDKv2 implementation, but it causes a data inconsistency that the newer Plugin Framework SDK doesn't like.
If this resource were ever to be migrated over to PF, we'd have to split this out.
considering we don't know when that would happen, and that IcebergNamespace was done very similarly, I'm opting to leave this as is.
|
updated release note |
c2thorn
left a comment
There was a problem hiding this comment.
looks like there are still some unit test failures related to the metadata:
--- FAIL: TestValidateResourceMetadata (0.61s)
resource_inventory_test.go:312: resource_biglake_iceberg_table_generated_meta.yaml: partition_spec.fields.name: `field` must be omitted because it can be inferred from `api_field`
resource_inventory_test.go:312: resource_biglake_iceberg_table_generated_meta.yaml: partition_spec.fields.transform: `field` must be omitted because it can be inferred from `api_field`
resource_inventory_test.go:312: resource_biglake_iceberg_table_generated_meta.yaml: schema.schema_id: `field` must be omitted because it can be inferred from `api_field`
resource_inventory_test.go:312: resource_biglake_iceberg_catalog_generated_meta.yaml: catalog_type: `field` must be omitted because it can be inferred from `api_field`
resource_inventory_test.go:312: resource_biglake_iceberg_catalog_generated_meta.yaml: create_time: `field` must be omitted because it can be inferred from `api_field`
resource_inventory_test.go:312: resource_biglake_iceberg_catalog_generated_meta.yaml: credential_mode: `field` must be omitted because it can be inferred from `api_field`
resource_inventory_test.go:312: resource_biglake_iceberg_catalog_generated_meta.yaml: default_location: `field` must be omitted because it can be inferred from `api_field`
resource_inventory_test.go:312: resource_biglake_iceberg_catalog_generated_meta.yaml: storage_regions: `field` must be omitted because it can be inferred from `api_field`
resource_inventory_test.go:312: resource_biglake_iceberg_catalog_generated_meta.yaml: update_time: `field` must be omitted because it can be inferred from `api_field`
c2thorn
left a comment
There was a problem hiding this comment.
unit test checks need to pass
|
Hi there, I'm the Modular magician. I've detected the following information about your changes: Diff reportYour PR generated some diffs in downstreams - here they are.
|
|
@c2thorn changes made! It's looking like part of this is a mmv1 bug. |
|
Hi there, I'm the Modular magician. I've detected the following information about your changes: Diff reportYour PR generated some diffs in downstreams - here they are.
|
Non-exercised tests🔴 Tests were added that are skipped in VCR:
Tests analyticsTotal tests: 6141 Click here to see the affected service packages
Action takenFound 2 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests
|
|
🟢 Tests passed during RECORDING mode: 🔴 Tests failed when rerunning REPLAYING mode: Tests failed due to non-determinism or randomness when the VCR replayed the response after the HTTP request was made. Please fix these to complete your PR. If you believe these test failures to be incorrect or unrelated to your change, or if you have any questions, please raise the concern with your reviewer. 🟢 All tests passed! |
Non-exercised tests🔴 Tests were added that are skipped in VCR:
Tests analyticsTotal tests: 6141 Click here to see the affected service packages
Action takenFound 2 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests
|
|
🟢 Tests passed during RECORDING mode: 🔴 Tests failed when rerunning REPLAYING mode: Tests failed due to non-determinism or randomness when the VCR replayed the response after the HTTP request was made. Please fix these to complete your PR. If you believe these test failures to be incorrect or unrelated to your change, or if you have any questions, please raise the concern with your reviewer. 🟢 All tests passed! |
| tmp = regexp.MustCompile(`([a-z\d])([A-Z])`).ReplaceAllString(tmp, "${1}_${2}") | ||
| tmp = strings.Replace(tmp, "-", "_", 1) | ||
| tmp = strings.Replace(tmp, ".", "_", 1) | ||
| tmp = strings.ReplaceAll(tmp, "-", "_") |
There was a problem hiding this comment.
looks like this didn't have any non-iceberg side effects
cfd244c
Release Note Template for Downstream PRs (will be copied)
See Write release notes for guidance.