Added dlp ContentPolicy resource - #18886
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
|
@modular-magician reassign-reviewer |
|
Googlers: For automatic test runs see go/terraform-auto-test-runs. @SirGitsalot, 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. |
0e4f803 to
eb8b4d8
Compare
|
Hi there, I'm the Modular magician. I've detected the following information about your changes for commit 0e4f803: Diff reportYour PR generated the following diffs in downstream repositories:
Test reportAnalytics
Affected Service Packages
Step 1: Replaying Mode 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
View the replaying VCR build log Step 2: Recording Mode
🟢 All tests passed! View the recording VCR build log or the debug logs folder for detailed results. @melinath, @annadata, @SirGitsalot VCR tests complete for 0e4f803! |
|
Hi there, I'm the Modular magician. I've detected the following information about your changes for commit eb8b4d8: Diff reportYour PR generated the following diffs in downstream repositories:
Test reportAnalytics
Affected Service Packages
Step 1: Replaying Mode 🟢 All tests passed in Replaying mode! No Recording was needed. View the replaying VCR build log @melinath, @annadata, @SirGitsalot VCR tests complete for eb8b4d8! |
|
GA Test run succeeded - this is ready for review now. Sorry for the false start earlier! |
| delete_url: '{{parent}}/contentPolicies/{{name}}' | ||
| update_mask: true | ||
| update_verb: PATCH | ||
| id_format: '{{parent}}/contentPolicies/{{name}}' |
There was a problem hiding this comment.
Without an explicit import_format, Magic Modules defaults to documenting both {{parent}}/contentPolicies/{{name}} and {{parent}}/{{name}}.
Because dlp_content_policy_import.go.tmpl expects a 6-part ID, running terraform import ... {{parent}}/{{name}} (5 parts) will fail with Unexpected import id.
Explicitly declaring import_format ensures the generated documentation only advertises the supported format:
| id_format: '{{parent}}/contentPolicies/{{name}}' | |
| id_format: '{{parent}}/contentPolicies/{{name}}' | |
| import_format: | |
| - '{{parent}}/contentPolicies/{{name}}' |
| - name: status | ||
| type: NestedObject | ||
| description: Detailed error codes and messages. | ||
| output: true | ||
| properties: | ||
| - name: code | ||
| type: Integer | ||
| description: The status code, which should be an enum value of google.rpc.Code. | ||
| output: true | ||
| - name: message | ||
| type: String | ||
| description: A developer-facing error message, which should be in English. | ||
| output: true | ||
| - name: details | ||
| type: Array | ||
| description: A list of messages that carry the error details. | ||
| output: true | ||
| custom_flatten: templates/terraform/custom_flatten/dlp_discovery_config_error_details.tmpl | ||
| item_type: | ||
| type: KeyValuePairs | ||
| - name: timestamps | ||
| type: Array | ||
| description: The times the error occurred. List includes the oldest timestamp and the last 9 timestamps. | ||
| output: true | ||
| item_type: | ||
| type: String | ||
| - name: details | ||
| type: String | ||
| description: Extra details for users to understand what went wrong. | ||
| output: true |
There was a problem hiding this comment.
In the backend proto definition:
message Error {
// Detailed error codes and messages.
google.rpc.Status details = 1;
// The times the error occurred. List includes the oldest timestamp and the
// last 9 timestamps.
repeated google.protobuf.Timestamp timestamps = 2;
// Additional information about the error.
ErrorExtraInfo extra_info = 4;
}The google.rpc.Status message is returned in the field named details, not status.
Because this schema names the nested object status and adds a string property details (Extra details for users to understand what went wrong), two issues occur:
original["status"]will always benilsince the API response returns{"details": {"code": ..., "message": ..., "details": [...]}}.- The flattener for the string
detailsfield will receiveoriginal["details"](which is a mapmap[string]interface{}) and try to set it into aschema.TypeString, which will fail with a schema type mismatch / panic at runtime when errors are returned.
Following the pattern in DiscoveryConfig.yaml, we should rename status to details and remove the string details (or map extra_info to extraInfo as an Enum):
| - name: status | |
| type: NestedObject | |
| description: Detailed error codes and messages. | |
| output: true | |
| properties: | |
| - name: code | |
| type: Integer | |
| description: The status code, which should be an enum value of google.rpc.Code. | |
| output: true | |
| - name: message | |
| type: String | |
| description: A developer-facing error message, which should be in English. | |
| output: true | |
| - name: details | |
| type: Array | |
| description: A list of messages that carry the error details. | |
| output: true | |
| custom_flatten: templates/terraform/custom_flatten/dlp_discovery_config_error_details.tmpl | |
| item_type: | |
| type: KeyValuePairs | |
| - name: timestamps | |
| type: Array | |
| description: The times the error occurred. List includes the oldest timestamp and the last 9 timestamps. | |
| output: true | |
| item_type: | |
| type: String | |
| - name: details | |
| type: String | |
| description: Extra details for users to understand what went wrong. | |
| output: true | |
| - name: details | |
| type: NestedObject | |
| description: Detailed error codes and messages. | |
| output: true | |
| properties: | |
| - name: code | |
| type: Integer | |
| description: The status code, which should be an enum value of google.rpc.Code. | |
| output: true | |
| - name: message | |
| type: String | |
| description: A developer-facing error message, which should be in English. | |
| output: true | |
| - name: details | |
| type: Array | |
| description: A list of messages that carry the error details. | |
| output: true | |
| custom_flatten: templates/terraform/custom_flatten/dlp_discovery_config_error_details.tmpl | |
| item_type: | |
| type: KeyValuePairs | |
| - name: timestamps | |
| type: Array | |
| description: The times the error occurred. List includes the oldest timestamp and the last 9 timestamps. | |
| output: true | |
| item_type: | |
| type: String | |
| - name: extraInfo | |
| type: Enum | |
| description: Additional information about the error. | |
| output: true | |
| enum_values: | |
| - IMAGE_SCAN_UNAVAILABLE_IN_REGION | |
| - FILE_STORE_CLUSTER_UNSUPPORTED |
| if len(parts) == 6 { | ||
| if err := d.Set("name", parts[5]); err != nil { | ||
| return nil, fmt.Errorf("Error setting name: %s", err) | ||
| } | ||
| } else if len(parts) == 4 { | ||
| if err := d.Set("name", parts[3]); err != nil { | ||
| return nil, fmt.Errorf("Error setting name: %s", err) | ||
| } | ||
| } else { | ||
| return nil, fmt.Errorf("Unexpected import id: %s, expected form {{"{{"}}parent{{"}}"}}/contentPolicies/{{"{{"}}name{{"}}"}}", d.Get("name").(string)) | ||
| } | ||
| // Remove "/contentPolicies/name" from the id | ||
| parts = parts[:len(parts)-2] | ||
| if err := d.Set("parent", strings.Join(parts, "/")); err != nil { | ||
| return nil, fmt.Errorf("Error setting parent: %s", err) | ||
| } | ||
|
|
||
| // Replace import id for the resource id | ||
| id, err := tpgresource.ReplaceVars(d, config, "{{"{{"}}parent{{"}}"}}/contentPolicies/{{"{{"}}name{{"}}"}}") | ||
| if err != nil { | ||
| return nil, fmt.Errorf("Error constructing id: %s", err) | ||
| } |
There was a problem hiding this comment.
A few cleanup suggestions for the custom import template:
ContentPolicyis strictly a regional resource in the API (projects/{project}/locations/{location}/contentPolicies/{name}, 6 parts). Theelse if len(parts) == 4branch (inherited fromdlp_import.go.tmpl) accepts an invalid parent (projects/{project}) without a location.- We can also verify that
parts[4] == "contentPolicies"to guard against importing IDs belonging to other resource collections. - Standardize Go error messages to start with lowercase and wrap errors using
%w.
| if len(parts) == 6 { | |
| if err := d.Set("name", parts[5]); err != nil { | |
| return nil, fmt.Errorf("Error setting name: %s", err) | |
| } | |
| } else if len(parts) == 4 { | |
| if err := d.Set("name", parts[3]); err != nil { | |
| return nil, fmt.Errorf("Error setting name: %s", err) | |
| } | |
| } else { | |
| return nil, fmt.Errorf("Unexpected import id: %s, expected form {{"{{"}}parent{{"}}"}}/contentPolicies/{{"{{"}}name{{"}}"}}", d.Get("name").(string)) | |
| } | |
| // Remove "/contentPolicies/name" from the id | |
| parts = parts[:len(parts)-2] | |
| if err := d.Set("parent", strings.Join(parts, "/")); err != nil { | |
| return nil, fmt.Errorf("Error setting parent: %s", err) | |
| } | |
| // Replace import id for the resource id | |
| id, err := tpgresource.ReplaceVars(d, config, "{{"{{"}}parent{{"}}"}}/contentPolicies/{{"{{"}}name{{"}}"}}") | |
| if err != nil { | |
| return nil, fmt.Errorf("Error constructing id: %s", err) | |
| } | |
| if len(parts) == 6 && parts[4] == "contentPolicies" { | |
| if err := d.Set("name", parts[5]); err != nil { | |
| return nil, fmt.Errorf("error setting name: %w", err) | |
| } | |
| } else { | |
| return nil, fmt.Errorf("unexpected import id: %s, expected form {{"{{"}}parent{{"}}"}}/contentPolicies/{{"{{"}}name{{"}}"}}", d.Get("name").(string)) | |
| } | |
| // Remove "/contentPolicies/name" from the id | |
| parts = parts[:len(parts)-2] | |
| if err := d.Set("parent", strings.Join(parts, "/")); err != nil { | |
| return nil, fmt.Errorf("error setting parent: %w", err) | |
| } | |
| // Replace import id for the resource id | |
| id, err := tpgresource.ReplaceVars(d, config, "{{"{{"}}parent{{"}}"}}/contentPolicies/{{"{{"}}name{{"}}"}}") | |
| if err != nil { | |
| return nil, fmt.Errorf("error constructing id: %w", err) | |
| } |
| project: PROJECT_NAME | ||
| location: REGION | ||
| parameters: | ||
| - name: parent |
There was a problem hiding this comment.
I could be convinced that this should remain in order to be consistent with other DLP resources, but: since ContentPolicy can only be parented by project and location then parent should be replaced by those two parameters instead.
DiscoveryConfig uses parent but it supports projects and organizations; InspectTemplate supports projects and organizations, with and without location; and JobTrigger supports projects with and without location.
3 participants
b/554176636
Release Note Template for Downstream PRs (will be copied)
See Write release notes for guidance.