Skip to content
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

Added job_notification_emails and deidentify actions in google_data_loss_prevention_job_trigger #7687

Conversation

abheda-crest
Copy link
Contributor

@abheda-crest abheda-crest commented Apr 11, 2023

Added job_notification_emails and deidentify actions in the google_data_loss_prevention_job_trigger resource.

If this PR is for Terraform, I acknowledge that I have:

  • Searched through the issue tracker for an open issue that this either resolves or contributes to, commented on it to claim it, and written "fixes {url}" or "part of {url}" in this PR description. If there were no relevant open issues, I opened one and commented that I would like to work on it (not necessary for very small changes).
  • Ensured that all new fields I added that can be set by a user appear in at least one example (for generated resources) or third_party test (for handwritten resources or update tests).
  • Generated Terraform providers, and ran make test and make lint in the generated providers to ensure it passes unit and linter tests.
  • Ran relevant acceptance tests using my own Google Cloud project and credentials (If the acceptance tests do not yet pass or you are unable to run them, please let your reviewer know).
  • Read the Release Notes Guide before writing my release note below.

Release Note Template for Downstream PRs (will be copied)

dlp: added `inspect_job.actions.job_notification_emails` and `inspect_job.actions.deidentify`  fields to `google_data_loss_prevention_job_trigger` resource

@abheda-crest abheda-crest requested a review from a team as a code owner April 11, 2023 08:53
@abheda-crest abheda-crest requested review from zli82016 and removed request for a team April 11, 2023 08:53
@modular-magician modular-magician added awaiting-approval Pull requests that need reviewer's approval to run presubmit tests and removed awaiting-approval Pull requests that need reviewer's approval to run presubmit tests labels Apr 11, 2023
@modular-magician
Copy link
Collaborator

Hi there, I'm the Modular magician. I've detected the following information about your changes:

Diff report

Your PR generated some diffs in downstreams - here they are.

Terraform GA: Diff ( 3 files changed, 647 insertions(+))
Terraform Beta: Diff ( 3 files changed, 647 insertions(+))
TF Conversion: Diff ( 3 files changed, 189 insertions(+), 3 deletions(-))

@modular-magician
Copy link
Collaborator

Tests analytics

Total tests: 2566
Passed tests 2279
Skipped tests: 275
Affected tests: 12

Action taken

Found 12 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests
TestAccDataLossPreventionJobTrigger_dlpJobTriggerDeidentify|TestAccDataLossPreventionJobTrigger_dlpJobTriggerUpdateExample2|TestAccDataLossPreventionJobTrigger_dlpJobTriggerSccOutputExample|TestAccDataLossPreventionJobTrigger_dlpJobTriggerDataCatalogOutputExample|TestAccDataLossPreventionJobTrigger_dlpJobTriggerBigqueryRowLimitPercentageExample|TestAccDataLossPreventionJobTrigger_dlpJobTriggerUpdateExample|TestAccDataLossPreventionJobTrigger_dlpJobTriggerBigqueryRowLimitExample|TestAccDataLossPreventionJobTrigger_dlpJobTriggerBasicExample|TestAccComputeForwardingRule_update|TestAccDataSourceGoogleFirebaseAndroidAppConfig|TestAccApigeeKeystoresAliasesKeyCertFile_apigeeKeystoresAliasesKeyCertFileTestExample|TestAccApigeeKeystoresAliasesPkcs12_ApigeeKeystoresAliasesPkcs12Example

Get to know how VCR tests work

@modular-magician
Copy link
Collaborator

Tests passed during RECORDING mode:
TestAccDataLossPreventionJobTrigger_dlpJobTriggerDeidentify[Debug log]
TestAccDataLossPreventionJobTrigger_dlpJobTriggerUpdateExample2[Debug log]
TestAccDataLossPreventionJobTrigger_dlpJobTriggerSccOutputExample[Debug log]
TestAccDataLossPreventionJobTrigger_dlpJobTriggerDataCatalogOutputExample[Debug log]
TestAccDataLossPreventionJobTrigger_dlpJobTriggerBigqueryRowLimitPercentageExample[Debug log]
TestAccDataLossPreventionJobTrigger_dlpJobTriggerUpdateExample[Debug log]
TestAccDataLossPreventionJobTrigger_dlpJobTriggerBigqueryRowLimitExample[Debug log]
TestAccDataLossPreventionJobTrigger_dlpJobTriggerBasicExample[Debug log]
TestAccComputeForwardingRule_update[Debug log]
TestAccDataSourceGoogleFirebaseAndroidAppConfig[Debug log]
TestAccApigeeKeystoresAliasesKeyCertFile_apigeeKeystoresAliasesKeyCertFileTestExample[Debug log]
TestAccApigeeKeystoresAliasesPkcs12_ApigeeKeystoresAliasesPkcs12Example[Debug log]

All tests passed
View the build log or the debug log for each test


If a file type is set in this field that isn't supported by the Deidentify action then the job will fail and will not be successfully created/started.
item_type: !ruby/object:Api::Type::Enum
name: 'undefined'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The name should be fileType?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for pointing that out. I've updated the name to fileType so that it is relevant as per the API docs. Just wanted to clarify that this name won't appear in the generated provider.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can click the link in Diff report to check the generated resource in the provider.

https://github.com/modular-magician/terraform-provider-google/compare/auto-pr-7687-old..auto-pr-7687

file_type is not there.

- deidentify
allow_empty_object: true
send_empty_value: true
properties: []
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just want to double check that the value of jobNotificationEmails should be {} ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the API docs, there is no information about the object of jobNotificationEmails. Moreover, I've tried sending jobNotificationEmails as {} and it's working fine. I've also checked this with the API explorer and it is not suggesting any fields inside jobNotificationEmails.

Following is the payload I've used to configure jobTriggers with jobNotificationEmails action:

---[ REQUEST ]---------------------------------------
{
 "jobTrigger": {
  "description": "Description for the job_trigger created by terraform",
  "displayName": "TerraformDisplayName",
  "inspectJob": {
   "actions": [
    {
     "jobNotificationEmails": {},
     "publishFindingsToCloudDataCatalog": null,
     "publishSummaryToCscc": null
    }
   ],
   "inspectTemplateName": "sample-inspect-template",
   "storageConfig": {
    "cloudStorageOptions": {
     "fileSet": {
      "url": "gs://mybucket/directory/"
     }
    }
   }
  },
  "status": "HEALTHY",
  "triggers": [
   {
    "schedule": {
     "recurrencePeriodDuration": "86400s"
    }
   }
  ]
 }
}

---[ RESPONSE ]--------------------------------------
{
  "name": "projects/{project_name}/jobTriggers/{job_trigger_id}",
  "displayName": "TerraformDisplayName",
  "description": "Description for the job_trigger created by terraform",
  "inspectJob": {
    "storageConfig": {
      "cloudStorageOptions": {
        "fileSet": {
          "url": "gs://mybucket/directory/"
        }
      }
    },
    "inspectTemplateName": "sample-inspect-template",
    "actions": [
      {
        "jobNotificationEmails": {}
      }
    ]
  },
  "triggers": [
    {
      "schedule": {
        "recurrencePeriodDuration": "86400s"
      }
    }
  ],
  "createTime": "2023-04-13T08:22:50.200795Z",
  "updateTime": "2023-04-13T08:22:50.200795Z",
  "status": "HEALTHY"
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I cannot find the doc for this field, either. I asked the service team and am waiting for their response. Sorry for holding the PR a little bit.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I checked with the service team and they confirmed that this field should be an empty json object.

@@ -101,6 +101,57 @@ func TestAccDataLossPreventionJobTrigger_dlpJobTriggerPubsub(t *testing.T) {
})
}

func TestAccDataLossPreventionJobTrigger_dlpJobTriggerJobNotificationEmails(t *testing.T) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you please move these two new tests to examples in JobTrigger.yaml file? https://github.com/GoogleCloudPlatform/magic-modules/blob/main/mmv1/products/dlp/JobTrigger.yaml#L28

In this file mmv1/third_party/terraform/tests/resource_data_loss_prevention_job_trigger_test.go, can you please add tests for updating field?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've moved these tests to examples and also added the update tests.

@modular-magician modular-magician added the awaiting-approval Pull requests that need reviewer's approval to run presubmit tests label Apr 13, 2023
@JayS-crest
Copy link
Contributor

Hey @zli82016, I've worked on a PR to further extend same resource and have Sam Levenick as reviewer and it seems like he's not available. Could you please review that PR? Here's the link: #7677

@modular-magician modular-magician removed the awaiting-approval Pull requests that need reviewer's approval to run presubmit tests label Apr 13, 2023
@zli82016
Copy link
Member

Hey @zli82016, I've worked on a PR to further extend same resource and have Sam Levenick as reviewer and it seems like he's not available. Could you please review that PR? Here's the link: #7677

Sure. I will review the PR #7677 later.

@modular-magician
Copy link
Collaborator

Hi there, I'm the Modular magician. I've detected the following information about your changes:

Diff report

Your PR generated some diffs in downstreams - here they are.

Terraform GA: Diff ( 4 files changed, 1052 insertions(+))
Terraform Beta: Diff ( 4 files changed, 1052 insertions(+))
TF Conversion: Diff ( 3 files changed, 189 insertions(+), 3 deletions(-))

@modular-magician
Copy link
Collaborator

Tests analytics

Total tests: 2618
Passed tests 2334
Skipped tests: 277
Affected tests: 7

Action taken

Found 7 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests
TestAccDataLossPreventionJobTrigger_dlpJobTriggerChangingActions|TestAccDataLossPreventionJobTrigger_dlpJobTriggerDeidentifyUpdate|TestAccDataLossPreventionJobTrigger_dlpJobTriggerDeidentifyExample|TestAccDataLossPreventionJobTrigger_dlpJobTriggerJobNotificationEmailsExample|TestAccApigeeKeystoresAliasesPkcs12_ApigeeKeystoresAliasesPkcs12Example|TestAccDataSourceGoogleFirebaseAndroidAppConfig|TestAccApigeeKeystoresAliasesKeyCertFile_apigeeKeystoresAliasesKeyCertFileTestExample

Get to know how VCR tests work

@modular-magician
Copy link
Collaborator

Tests passed during RECORDING mode:
TestAccDataLossPreventionJobTrigger_dlpJobTriggerChangingActions[Debug log]
TestAccDataLossPreventionJobTrigger_dlpJobTriggerDeidentifyUpdate[Debug log]
TestAccDataLossPreventionJobTrigger_dlpJobTriggerDeidentifyExample[Debug log]
TestAccDataLossPreventionJobTrigger_dlpJobTriggerJobNotificationEmailsExample[Debug log]
TestAccApigeeKeystoresAliasesPkcs12_ApigeeKeystoresAliasesPkcs12Example[Debug log]
TestAccDataSourceGoogleFirebaseAndroidAppConfig[Debug log]
TestAccApigeeKeystoresAliasesKeyCertFile_apigeeKeystoresAliasesKeyCertFileTestExample[Debug log]

All tests passed
View the build log or the debug log for each test

Copy link
Member

@zli82016 zli82016 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@zli82016 zli82016 merged commit 943e8d4 into GoogleCloudPlatform:main Apr 13, 2023
ericayyliu pushed a commit to ericayyliu/magic-modules that referenced this pull request Jul 26, 2023
…oss_prevention_job_trigger (GoogleCloudPlatform#7687)

* Added job_notification_emails and deidentify actions in google_data_loss_prevention_job_trigger

* Updated the deidentify actions enum and added update tests for deidentify action
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants