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 backup support for google_filestore_instance #6742

Merged

Conversation

googlerjk
Copy link
Contributor

@googlerjk googlerjk commented Oct 26, 2022

Added resource google_filestore_backup
Added sourceBackup field to google_filestore_instance resource
fixes hashicorp/terraform-provider-google#7655

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).
  • Generated Terraform, and ran make test and make lint to ensure it passes unit and linter tests.
  • 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).
  • Ran relevant acceptance tests (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)

`google_filestore_backup`

@googlerjk googlerjk requested a review from c2thorn October 26, 2022 09:39
@googlerjk googlerjk self-assigned this Oct 26, 2022
@googlerjk googlerjk changed the title Jk adding filestore backup support Added backup support for google_filestore_instance Oct 28, 2022
mmv1/products/filestore/api.yaml Outdated Show resolved Hide resolved
mmv1/products/redis/api.yaml Outdated Show resolved Hide resolved
mmv1/products/filestore/api.yaml Outdated Show resolved Hide resolved
mmv1/products/filestore/api.yaml Outdated Show resolved Hide resolved
@googlerjk googlerjk force-pushed the jk-adding-filestore-backup-support branch from ffb2671 to 8ceed51 Compare November 10, 2022 21:21
@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 ( 8 files changed, 964 insertions(+), 4 deletions(-))
Terraform Beta: Diff ( 8 files changed, 964 insertions(+), 4 deletions(-))
TF Validator: Diff ( 4 files changed, 104 insertions(+), 3 deletions(-))
TF OiCS: Diff ( 5 files changed, 142 insertions(+), 1 deletion(-))

@modular-magician
Copy link
Collaborator

Tests analytics

Total tests: 2236
Passed tests 1993
Skipped tests: 241
Failed tests: 2

Action taken

Triggering VCR tests in RECORDING mode for the tests that failed during VCR. Click here to see the failed tests
TestAccFilestoreInstance_filestoreInstanceFullExample|TestAccFilestoreBackup_filestoreBackupFullExample

@googlerjk googlerjk force-pushed the jk-adding-filestore-backup-support branch from 14249b8 to 6eda6d6 Compare November 10, 2022 23:26
@googlerjk
Copy link
Contributor Author

/gcbrun

@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 ( 8 files changed, 964 insertions(+), 4 deletions(-))
Terraform Beta: Diff ( 8 files changed, 964 insertions(+), 4 deletions(-))
TF Validator: Diff ( 4 files changed, 104 insertions(+), 3 deletions(-))
TF OiCS: Diff ( 5 files changed, 142 insertions(+), 1 deletion(-))

@modular-magician
Copy link
Collaborator

Tests analytics

Total tests: 2244
Passed tests 2000
Skipped tests: 241
Failed tests: 3

Action taken

Triggering VCR tests in RECORDING mode for the tests that failed during VCR. Click here to see the failed tests
TestAccFilestoreInstance_filestoreInstanceFullExample|TestAccFilestoreBackup_filestoreBackupFullExample|TestAccComputeForwardingRule_update

@modular-magician
Copy link
Collaborator

Tests passed during RECORDING mode:
TestAccComputeForwardingRule_update[Debug log]

Tests failed during RECORDING mode:
TestAccFilestoreInstance_filestoreInstanceFullExample[Error message] [Debug log]
TestAccFilestoreBackup_filestoreBackupFullExample[Error message] [Debug log]

Please fix these to complete your PR
View the build log or the debug log for each test

@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 ( 8 files changed, 994 insertions(+), 4 deletions(-))
Terraform Beta: Diff ( 8 files changed, 994 insertions(+), 4 deletions(-))
TF Validator: Diff ( 4 files changed, 104 insertions(+), 3 deletions(-))
TF OiCS: Diff ( 5 files changed, 157 insertions(+), 1 deletion(-))

@modular-magician
Copy link
Collaborator

Tests analytics

Total tests: 2255
Passed tests 2011
Skipped tests: 241
Failed tests: 3

Action taken

Triggering VCR tests in RECORDING mode for the tests that failed during VCR. Click here to see the failed tests
TestAccComputeForwardingRule_update|TestAccFilestoreInstance_filestoreInstanceFullExample|TestAccFilestoreBackup_filestoreBackupBasicExample

@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 ( 8 files changed, 994 insertions(+), 4 deletions(-))
Terraform Beta: Diff ( 8 files changed, 994 insertions(+), 4 deletions(-))
TF Validator: Diff ( 4 files changed, 104 insertions(+), 3 deletions(-))
TF OiCS: Diff ( 5 files changed, 157 insertions(+), 1 deletion(-))

@modular-magician
Copy link
Collaborator

Tests analytics

Total tests: 2255
Passed tests 2010
Skipped tests: 241
Failed tests: 4

Action taken

Triggering VCR tests in RECORDING mode for the tests that failed during VCR. Click here to see the failed tests
TestAccFirebaserulesRelease_BasicRelease|TestAccFilestoreBackup_filestoreBackupBasicExample|TestAccFilestoreInstance_filestoreInstanceFullExample|TestAccComputeForwardingRule_update

@modular-magician
Copy link
Collaborator

Tests passed during RECORDING mode:
TestAccFirebaserulesRelease_BasicRelease[Debug log]
TestAccComputeForwardingRule_update[Debug log]

Tests failed during RECORDING mode:
TestAccFilestoreBackup_filestoreBackupBasicExample[Error message] [Debug log]
TestAccFilestoreInstance_filestoreInstanceFullExample[Error message] [Debug log]

Please fix these to complete your PR
View the build log or the debug log for each test

@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 ( 8 files changed, 994 insertions(+), 4 deletions(-))
Terraform Beta: Diff ( 8 files changed, 994 insertions(+), 4 deletions(-))
TF Validator: Diff ( 4 files changed, 104 insertions(+), 3 deletions(-))
TF OiCS: Diff ( 5 files changed, 157 insertions(+), 1 deletion(-))

@modular-magician
Copy link
Collaborator

Tests analytics

Total tests: 2257
Passed tests 2013
Skipped tests: 241
Failed tests: 3

Action taken

Triggering VCR tests in RECORDING mode for the tests that failed during VCR. Click here to see the failed tests
TestAccFilestoreBackup_filestoreBackupBasicExample|TestAccFilestoreInstance_filestoreInstanceFullExample|TestAccFirebaserulesRelease_BasicRelease

@modular-magician
Copy link
Collaborator

Tests passed during RECORDING mode:
TestAccFirebaserulesRelease_BasicRelease[Debug log]

Tests failed during RECORDING mode:
TestAccFilestoreBackup_filestoreBackupBasicExample[Error message] [Debug log]
TestAccFilestoreInstance_filestoreInstanceFullExample[Error message] [Debug log]

Please fix these to complete your PR
View the build log or the debug log for each test

@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 ( 8 files changed, 968 insertions(+), 4 deletions(-))
Terraform Beta: Diff ( 8 files changed, 968 insertions(+), 4 deletions(-))
TF Validator: Diff ( 4 files changed, 104 insertions(+), 3 deletions(-))
TF OiCS: Diff ( 5 files changed, 144 insertions(+), 1 deletion(-))

@modular-magician
Copy link
Collaborator

Tests analytics

Total tests: 2260
Passed tests 2017
Skipped tests: 241
Failed tests: 2

Action taken

Triggering VCR tests in RECORDING mode for the tests that failed during VCR. Click here to see the failed tests
TestAccFilestoreInstance_filestoreInstanceFullExample|TestAccFilestoreBackup_filestoreBackupBasicExample

@modular-magician
Copy link
Collaborator

Tests failed during RECORDING mode:
TestAccFilestoreInstance_filestoreInstanceFullExample[Error message] [Debug log]
TestAccFilestoreBackup_filestoreBackupBasicExample[Error message] [Debug log]

Please fix these to complete your PR
View the build log or the debug log for each test

@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 ( 8 files changed, 995 insertions(+), 4 deletions(-))
Terraform Beta: Diff ( 8 files changed, 995 insertions(+), 4 deletions(-))
TF Validator: Diff ( 4 files changed, 114 insertions(+), 3 deletions(-))
TF OiCS: Diff ( 5 files changed, 144 insertions(+), 1 deletion(-))

@modular-magician
Copy link
Collaborator

Tests analytics

Total tests: 2268
Passed tests 2025
Skipped tests: 241
Failed tests: 2

Action taken

Triggering VCR tests in RECORDING mode for the tests that failed during VCR. Click here to see the failed tests
TestAccFilestoreBackup_filestoreBackupBasicExample|TestAccFilestoreInstance_filestoreInstanceFullExample

@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 ( 8 files changed, 997 insertions(+), 6 deletions(-))
Terraform Beta: Diff ( 8 files changed, 997 insertions(+), 6 deletions(-))
TF Validator: Diff ( 4 files changed, 114 insertions(+), 3 deletions(-))
TF OiCS: Diff ( 5 files changed, 145 insertions(+), 2 deletions(-))

@modular-magician
Copy link
Collaborator

Tests analytics

Total tests: 2268
Passed tests 2025
Skipped tests: 241
Failed tests: 2

Action taken

Triggering VCR tests in RECORDING mode for the tests that failed during VCR. Click here to see the failed tests
TestAccFilestoreInstance_filestoreInstanceFullExample|TestAccFilestoreBackup_filestoreBackupBasicExample

@modular-magician
Copy link
Collaborator

Tests failed during RECORDING mode:
TestAccFilestoreInstance_filestoreInstanceFullExample[Error message] [Debug log]
TestAccFilestoreBackup_filestoreBackupBasicExample[Error message] [Debug log]

Please fix these to complete your PR
View the build log or the debug log for each test

googlerjk and others added 11 commits December 7, 2022 00:08
Co-authored-by: Cameron Thornton <[email protected]>

Update mmv1/products/redis/api.yaml

Co-authored-by: Cameron Thornton <[email protected]>

Update mmv1/products/filestore/api.yaml

Co-authored-by: Cameron Thornton <[email protected]>

validator files removed, updates for filestore source backup field

quota lowered and label fixed

rm api.yaml

api.yaml added
@c2thorn
Copy link
Member

c2thorn commented Dec 7, 2022

I tested it locally and it doesn't seem the backup name is returned for the field source_backup in google_filestore_instance. Filestore instance and backup got created successfully though. Do you happen to know why it's not got returned?

It's only returned when an instance is restored from a backup, which won't be possible to do in TF without using the custom "restore" endpoint. Julius and I confirmed the field is returned as expected from the API by manually restoring an instance. Given that this is an output-only field, I feel confident enough with that manual check.

Copy link
Member

@c2thorn c2thorn left a comment

Choose a reason for hiding this comment

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

My apologies for missing this up to this point, but we really need to add a test for updatability. Can you write a handwritten test to check the updatability of description and labels ?

mmv1/products/filestore/api.yaml Show resolved Hide resolved
@googlerjk
Copy link
Contributor Author

@googlerjk The latest commit does not seem to address all the comments @c2thorn left. Is this the latest version? Sorry if I have missed anything. Comments that need to be addressed:

  1. what's the purpose of modifying example at mmv1/templates/terraform/examples/filestore_instance_full.tf.erb? The goal of that example is to test the google_filestore_instance resource
  2. input: true is added to nfsExportOptions in google_filestore_instance resource? Is that intentional?

this has been applied.

@googlerjk googlerjk force-pushed the jk-adding-filestore-backup-support branch from fbdfacd to 785c629 Compare December 8, 2022 21:59
@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 ( 8 files changed, 1082 insertions(+), 2 deletions(-))
Terraform Beta: Diff ( 8 files changed, 1082 insertions(+), 2 deletions(-))
TF Validator: Diff ( 4 files changed, 114 insertions(+), 3 deletions(-))
TF OiCS: Diff ( 4 files changed, 131 insertions(+))

@modular-magician
Copy link
Collaborator

Tests analytics

Total tests: 2336
Passed tests 2089
Skipped tests: 244
Failed tests: 3

Action taken

Triggering VCR tests in RECORDING mode for the tests that failed during VCR. Click here to see the failed tests
TestAccFirebaserulesRelease_BasicRelease|TestAccFilestoreBackup_update|TestAccLoggingBucketConfigProject_cmekSettings

@modular-magician
Copy link
Collaborator

Tests passed during RECORDING mode:
TestAccFirebaserulesRelease_BasicRelease[Debug log]
TestAccFilestoreBackup_update[Debug log]
TestAccLoggingBucketConfigProject_cmekSettings[Debug log]

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

@googlerjk
Copy link
Contributor Author

My apologies for missing this up to this point, but we really need to add a test for updatability. Can you write a handwritten test to check the updatability of description and labels ?

Update made and pushed

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.

[Feature Request] Add the ability to add Filestore instance backups via Terraform
4 participants