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

Netapp active dirtectory #9546

Merged

Conversation

paavan-gopala-reddy
Copy link
Contributor

@paavan-gopala-reddy paavan-gopala-reddy commented Nov 30, 2023

Fixes - hashicorp/terraform-provider-google#16633

This PR is to support provisioning NetApp Active Directory Policy for Google Cloud NetApp Volumes through GCP terraform provider.

Release Note Template for Downstream PRs (will be copied)

`google_netapp_active_directory`

Copy link

google-cla bot commented Nov 30, 2023

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@modular-magician
Copy link
Collaborator

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

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

@modular-magician modular-magician added the awaiting-approval Pull requests that needs reviewer's approval to run presubmit tests label Nov 30, 2023
@paavan-gopala-reddy
Copy link
Contributor Author

Hi All, I have signed the CLA.

@modular-magician modular-magician removed the awaiting-approval Pull requests that needs reviewer's approval to run presubmit tests label Dec 1, 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 ( 5 files changed, 1446 insertions(+), 2 deletions(-))
Terraform Beta: Diff ( 5 files changed, 1446 insertions(+), 2 deletions(-))
TF Conversion: Diff ( 1 file changed, 236 insertions(+))
TF OiCS: Diff ( 4 files changed, 110 insertions(+))

Missing test report

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

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

resource "google_netapp_active_directory" "primary" {
  aes_encryption         = # value needed
  backup_operators       = # value needed
  description            = # value needed
  encrypt_dc_connections = # value needed
  kdc_hostname           = # value needed
  kdc_ip                 = # value needed
  labels                 = # value needed
  ldap_signing           = # value needed
  nfs_users_with_ldap    = # value needed
  organizational_unit    = # value needed
  security_operators     = # value needed
  site                   = # value needed
}

@modular-magician
Copy link
Collaborator

Tests analytics

Total tests: 3256
Passed tests 2923
Skipped tests: 331
Affected tests: 2

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
TestAccLoggingProjectSink_updatePreservesCustomWriter|TestAccNetappactiveDirectory_activeDirectoryCreateExample

Get to know how VCR tests work

@modular-magician
Copy link
Collaborator

$\textcolor{red}{\textsf{Tests failed during RECORDING mode:}}$
TestAccLoggingProjectSink_updatePreservesCustomWriter[Error message] [Debug log]
TestAccNetappactiveDirectory_activeDirectoryCreateExample[Error message] [Debug log]

$\textcolor{red}{\textsf{Please fix these to complete your PR.}}$
View the build log or the debug log for each test

Copy link
Contributor

@NickElliot NickElliot left a comment

Choose a reason for hiding this comment

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

Hi, did a first pass of things that need to be cleaned up!

mmv1/products/netapp/activeDirectory.yaml Outdated Show resolved Hide resolved
mmv1/products/netapp/activeDirectory.yaml Show resolved Hide resolved
@modular-magician modular-magician added the awaiting-approval Pull requests that needs reviewer's approval to run presubmit tests label Dec 5, 2023
@paavan-gopala-reddy
Copy link
Contributor Author

I see the request for adding the acceptance test for the following:

resource "google_netapp_active_directory" "primary" {
aes_encryption = # value needed
backup_operators = # value needed
description = # value needed
encrypt_dc_connections = # value needed
kdc_hostname = # value needed
kdc_ip = # value needed
labels = # value needed
ldap_signing = # value needed
nfs_users_with_ldap = # value needed
organizational_unit = # value needed
security_operators = # value needed
site = # value needed
}

Have few questions regarding the same.

a) Where exactly should the file reside?
b) What should be the file name?
c) And should we provide generic values for the above requested fields? Please confirm.

@modular-magician modular-magician removed the awaiting-approval Pull requests that needs reviewer's approval to run presubmit tests label Dec 5, 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 ( 5 files changed, 1439 insertions(+), 2 deletions(-))
Terraform Beta: Diff ( 5 files changed, 1439 insertions(+), 2 deletions(-))
TF Conversion: Diff ( 1 file changed, 236 insertions(+))
TF OiCS: Diff ( 4 files changed, 110 insertions(+))

Missing test report

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

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

resource "google_netapp_active_directory" "primary" {
  aes_encryption         = # value needed
  backup_operators       = # value needed
  description            = # value needed
  encrypt_dc_connections = # value needed
  kdc_hostname           = # value needed
  kdc_ip                 = # value needed
  labels                 = # value needed
  ldap_signing           = # value needed
  nfs_users_with_ldap    = # value needed
  organizational_unit    = # value needed
  security_operators     = # value needed
  site                   = # value needed
}

@modular-magician
Copy link
Collaborator

Tests analytics

Total tests: 3262
Passed tests 2929
Skipped tests: 332
Affected tests: 1

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
TestAccNetappactiveDirectory_activeDirectoryCreateExample

Get to know how VCR tests work

@modular-magician
Copy link
Collaborator

$\textcolor{green}{\textsf{Tests passed during RECORDING mode:}}$
TestAccNetappactiveDirectory_activeDirectoryCreateExample[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

Copy link
Contributor

@NickElliot NickElliot left a comment

Choose a reason for hiding this comment

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

Thanks for the quick updates!

a) Where exactly should the file reside?
b) What should be the file name?

The test(s) with these values can be added in more or less the same way as the first sample you added, just you need to create a resource that uses all of the fields rather than just the minimum ones needed you had in the first "basic" test. It should be named netapp_active_directory_full, or if there are conflicting fields that can't be set together, there could be more than one additional test with a naming scheme along the lines of netapp_active_directory_[withAfields]

c) And should we provide generic values for the above requested fields? Please confirm.

To go with that, there should not always be completely generic values -- if a resource is dependent on other ones to function, it will need to have those included in the sample (here in the development guide it shows an example of the subnetwork and network being included in the same test file). If it's a field that would need to be specific in actual use but isn't a value that would otherwise be represented in the terraform state via another resource, it's fine to include it as a generic along the lines of the previous "user" "pass".


Other than that, further down on the above linked instructions page is steps to add an update test -- that is also going to be necessary here, but thankfully it can be a pretty simple one! The goal is just to verify the resource can be updated in place -- it could be as simple as the first test step config is the same as your currently implemented basic test, while the 2nd changes net_bios_prefix to be "smbserver_update" (or something else that assumably passes validation from the API).

If there are any further questions or things I can help with, please ask away!

mmv1/products/netapp/activeDirectory.yaml Outdated Show resolved Hide resolved
mmv1/products/netapp/activeDirectory.yaml Outdated Show resolved Hide resolved
@modular-magician modular-magician added the awaiting-approval Pull requests that needs reviewer's approval to run presubmit tests label Dec 6, 2023
Copy link
Contributor

@okrause okrause left a comment

Choose a reason for hiding this comment

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

net_bios_prefix changed to <10 chars. LGTM

okrause

This comment was marked as duplicate.

@modular-magician modular-magician removed the awaiting-approval Pull requests that needs reviewer's approval to run presubmit tests label Dec 19, 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 ( 5 files changed, 1491 insertions(+), 2 deletions(-))
Terraform Beta: Diff ( 5 files changed, 1491 insertions(+), 2 deletions(-))
TF Conversion: Diff ( 1 file changed, 236 insertions(+))

@modular-magician modular-magician added the awaiting-approval Pull requests that needs reviewer's approval to run presubmit tests label Dec 19, 2023
@modular-magician
Copy link
Collaborator

Tests analytics

Total tests: 3305
Passed tests 2968
Skipped tests: 336
Affected tests: 1

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
TestAccNetappactiveDirectory_activeDirectory_FullUpdate

Get to know how VCR tests work

@modular-magician
Copy link
Collaborator

$\textcolor{green}{\textsf{Tests passed during RECORDING mode:}}$
TestAccNetappactiveDirectory_activeDirectory_FullUpdate[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

@modular-magician modular-magician removed the awaiting-approval Pull requests that needs reviewer's approval to run presubmit tests label Dec 19, 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 ( 5 files changed, 1455 insertions(+), 2 deletions(-))
Terraform Beta: Diff ( 5 files changed, 1455 insertions(+), 2 deletions(-))
TF Conversion: Diff ( 1 file changed, 236 insertions(+))

@modular-magician modular-magician added the awaiting-approval Pull requests that needs reviewer's approval to run presubmit tests label Dec 19, 2023
@modular-magician
Copy link
Collaborator

Tests analytics

Total tests: 3305
Passed tests 2968
Skipped tests: 336
Affected tests: 1

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
TestAccDataprocJobIamPolicy

Get to know how VCR tests work

@modular-magician
Copy link
Collaborator

$\textcolor{green}{\textsf{Tests passed during RECORDING mode:}}$
TestAccDataprocJobIamPolicy[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

@okrause
Copy link
Contributor

okrause commented Dec 19, 2023

Hey @NickElliot, can you please have a look? Thank you.

@modular-magician modular-magician removed the awaiting-approval Pull requests that needs reviewer's approval to run presubmit tests label Dec 19, 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 ( 5 files changed, 1454 insertions(+), 2 deletions(-))
Terraform Beta: Diff ( 5 files changed, 1454 insertions(+), 2 deletions(-))
TF Conversion: Diff ( 1 file changed, 236 insertions(+))

@modular-magician
Copy link
Collaborator

Tests analytics

Total tests: 3310
Passed tests 2974
Skipped tests: 336
Affected tests: 0

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

Copy link
Contributor

@NickElliot NickElliot left a comment

Choose a reason for hiding this comment

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

LGTM!

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