Netapp volumes: Add restoreParameters

[okrause]

This PR is the next stop on adding functionality to the google_netapp_volume resource. It adds support for restoreParameters which are two features:

1. Create a clone volume from snapshot of another volume
2. Create a volume from an backup

It also does some cleanup on comments and file renames for better consistency.

Release Note Template for Downstream PRs (will be copied)

netapp: added field `restore_parameters` and output-only fields `state`, `state_details` and `create_time` to `google_netapp_volume` resource

[modular-magician]

Hello! I am a robot. It looks like you are a: Community Contributor Googler Core Contributor. Tests will require approval to run.

@shuyama1, 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.

[okrause]

@shuyama1 Please take note. In a prior review you said we shouldn't add state, stateDetails and creationTime. Still true?

backupConfig part is commented out. I would prefer if we can handle it in a follow-on PR/merge.

[shuyama1]

@shuyama1 Please take note. In a prior review you said we shouldn't add state, stateDetails and creationTime. Still true?

backupConfig part is commented out. I would prefer if we can handle it in a follow-on PR/merge.

We used to find these output fields are typically not useful to Terraform users, but we start to see users request fields like creationTime recently, so feel free to add fields that are supported by the API and you think may be useful to users.

(Plus, I've updated changelog according to https://googlecloudplatform.github.io/magic-modules/contribute/release-notes/#write-release-notes)

[modular-magician]

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, 194 insertions(+), 34 deletions(-))
Terraform Beta: Diff ( 4 files changed, 194 insertions(+), 34 deletions(-))
TF Conversion: Diff ( 1 file changed, 40 insertions(+))
TF OiCS: Diff ( 4 files changed, 122 insertions(+))

Missing test report

Your PR includes resource fields which are not covered by any test.

Resource: google_netapp_volume (9 total tests)
Please add an acceptance test which includes these fields. The test should include the following:

resource "google_netapp_volume" "primary" {
  restore_parameters {
    source_backup = # value needed
  }
}

[modular-magician]

Tests analytics

Total tests: 12
Passed tests 10
Skipped tests: 0
Affected tests: 2

Click here to see the affected service packages

• netapp

Action taken

Found 2 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests

TestAccNetappVolume_netappVolumeBasicExample|TestAccNetappVolume_netappVolumeBasicExample_update

Get to know how VCR tests work

[modular-magician]

$\textcolor{green}{\textsf{Tests passed during RECORDING mode:}}$
TestAccNetappVolume_netappVolumeBasicExample[Debug log]
TestAccNetappVolume_netappVolumeBasicExample_update[Debug log]

Rerun these tests in REPLAYING mode to catch issues

$\textcolor{green}{\textsf{No issues found for passed tests after REPLAYING rerun.}}$

---

$\textcolor{green}{\textsf{All tests passed!}}$
View the build log or the debug log for each test

[shuyama1]

Can we set this in a test?

[okrause]

Not yet. We don't have a backup resource yet to create backups to restore from.
Backup is a bit complex. I have a draft for backup, which I didn't PR yet, since it needs more work. CRUD for backup isn't that complex itself, but the interaction between vault, backups and volumes is a bit complex. I want to make backupConfig parameter of volume work too first (making good progress, may need to update this PR to include it today or tomorrow). Also note that backup tests will extend test runtime for many minutes.

Back to this one: We currently cannot test it. I can remove it for now, but voted to keep it in, since the TF interaction is pretty simple. Specify an valid URN for a backup. There isn't too much which can go wrong in TF here, right?
What is your take: Omit it for now, or leave it in with the small risk that it might fail?

[shuyama1]

Sorry for the late reply.
Can you test it locally, like create a backup resource not via Terraform and use that for sourceBackup for volume creation via Terraform. I think we can keep it and local testing should be sufficient.

[okrause]

I tested it and it works as expected. They only fact worth mentioning: The API triggers the restore. The LRO returns after the restore job is set up. The new volume resource is being created with state==RESTORING, which will eventually (after minutes/hours/days, depending on the amount of data to restore) will transition to state=READY.
This means the restore operation triggered will run asynchronously and the will not wait for it to complete.

[modular-magician]

Tests analytics

Total tests: 12
Passed tests 11
Skipped tests: 0
Affected tests: 1

Click here to see the affected service packages

• netapp

Action taken

Found 1 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests

TestAccNetappVolume_netappVolumeBasicExample_update

Get to know how VCR tests work

[modular-magician]

$\textcolor{green}{\textsf{Tests passed during RECORDING mode:}}$
TestAccNetappVolume_netappVolumeBasicExample_update[Debug log]

Rerun these tests in REPLAYING mode to catch issues

$\textcolor{green}{\textsf{No issues found for passed tests after REPLAYING rerun.}}$

---

$\textcolor{green}{\textsf{All tests passed!}}$
View the build log or the debug log for each test

[shuyama1]

restoreParameters implementation looks good mostly. Only some small comments
Would you mind removing backupConfig related changes from this PR, including commented out implementation code and tests. Let's move it to a separate PR so we can ship this feature first if preferred

[shuyama1]

Any specific reason that we want to change this name + handwritten test names?

[okrause]

That's the inner Monk in me. I don't like the fact that the example isn't prefixed with netapp_volume to make clear where it belongs too. Having that prefix makes the content of the example folder more tidy, move the example file closer to other netapp_volume resources in the folder listing and tells anyone looking at the file name to which resource it belongs too.
So basically this is cleanup. Is this causing other problems I don't see?

[shuyama1]

No big issues. It's just we run integration tests nightly and changing name may lose history of test results. So just want to make sure this change is intended

[shuyama1]

/gcbrun

[modular-magician]

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, 420 insertions(+), 231 deletions(-))
Terraform Beta: Diff ( 4 files changed, 420 insertions(+), 231 deletions(-))
TF Conversion: Diff ( 1 file changed, 40 insertions(+))
TF OiCS: Diff ( 4 files changed, 122 insertions(+))

Missing test report

Your PR includes resource fields which are not covered by any test.

Resource: google_netapp_volume (9 total tests)
Please add an acceptance test which includes these fields. The test should include the following:

resource "google_netapp_volume" "primary" {
  restore_parameters {
    source_backup = # value needed
  }
}

[modular-magician]

Tests analytics

Total tests: 12
Passed tests 12
Skipped tests: 0
Affected tests: 0

Click here to see the affected service packages

• netapp

$\textcolor{green}{\textsf{All tests passed in REPLAYING mode.}}$
View the build log

[imrannayer]

@okrause is there any plan for adding backup_config block?

[okrause]

@imrannayer Yes, but that will be another PR. Waiting for a few dependencies to be resolved.

[shuyama1]

Thank you!

[shuyama1]

No big issues. It's just we run integration tests nightly and changing name may lose history of test results. So just want to make sure this change is intended

[okrause]

Thank you for the review and merging.