Added new Apigee resource /v1/organizations/<apigee_org>/environments/<apigee_env>/addonsConfig

[sharmajai09]

`google_apigee_environment_addons_config`

[github-actions]

Hello! I am a robot. Tests will require approval from a repository maintainer to run.

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

[melinath]

Taking this over as the original reviewer of #12417

[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.

google provider: Diff ( 5 files changed, 539 insertions(+), 2 deletions(-))
google-beta provider: Diff ( 5 files changed, 539 insertions(+), 2 deletions(-))
terraform-google-conversion: Diff ( 1 file changed, 79 insertions(+))

Missing test report

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

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

resource "google_apigee_environment_addons_config" "primary" {
  api_security_enabled = # value needed
}

[modular-magician]

Tests analytics

Total tests: 52
Passed tests: 17
Skipped tests: 35
Affected tests: 0

Click here to see the affected service packages

• apigee

#### Non-exercised tests

🔴 Tests were added that are skipped in VCR:

• TestAccApigeeEnvironmentAddonsConfig_apigeeEnvAddonsAnalyticsTestExample
  🟢 All tests passed!

View the build log

[melinath]

Manual test run: https://hashicorp.teamcity.com/buildConfiguration/TerraformProviders_GoogleCloud_GOOGLE_BETA_MMUPSTREAMTESTS_GOOGLEBETA_PACKAGE_APIGEE/288117

[melinath]

What would you think about sending a POST request to unset AddonEnablement on delete? This would match the behavior of AddonsConfig.

Suggested change

	exclude_delete: true
	delete_url: '{{env_id}}/addonsConfig:setAddonEnablement'
	delete_verb: 'POST'

[melinath]

Just want to double-check that this is desired; we usually only lengthen the timeouts, not shorten them.

[melinath]

(if delete support is added)

Suggested change

	actions: ['create', 'update']
	actions: ['create', 'update', 'delete']

[melinath]

(if delete support is added) you'll also need a test_check_destroy similar to AddonsConfig

magic-modules/mmv1/products/apigee/AddonsConfig.yaml

Line 43 in 55ec241

test_check_destroy: 'templates/terraform/custom_check_destroy/apigee_addons_override.go.tmpl'

[melinath]

There needs to be at least one docs example; you could add a doc-only example similar to AddonsConfig:

magic-modules/mmv1/products/apigee/AddonsConfig.yaml

Lines 45 to 46 in 55ec241

	- name: 'apigee_addons_basic'
	exclude_test: true

[melinath]

Just want to double-check that this needs to skip VCR tests? More information on the reasons for doing this (or not): https://googlecloudplatform.github.io/magic-modules/test/test/#skip-tests-in-vcr-replaying-mode

AddonsConfig tests don't seem to need this.

[melinath]

The addonsConfig part doesn't need to be in the base_url, since this isn't a "real" resource. Technically the base_url should probably be organizations/{{org_id}}/environments but I think given the overrides in place this might actually not get used at all.

Suggested change

	base_url: '{{env_id}}/addonsConfig'
	base_url: '{{env_id}}'

[melinath]

This doesn't need addonsConfig, since it's not a "real" resource - leaving it off better matches the AddonsConfig resource behavior.

Suggested change

	self_link: '{{env_id}}/addonsConfig'
	self_link: '{{env_id}}'

[melinath]

Suggested change

	if err := tpgresource.ParseImportId([]string{"(?P<env_id>.+)/addonsConfig"}, d, config); err != nil {
	if err := tpgresource.ParseImportId([]string{"(?P<env_id>.+)"}, d, config); err != nil {

[melinath]

Should be something like this:

Suggested change

	nameParts := strings.Split(d.Get("env_id").(string), "/")
	if len(nameParts) == 5 {
	id := fmt.Sprintf("organizations/%s/environments/%s", nameParts[1], nameParts[3])
	d.SetId(id)
	}
	id := d.Get("env_id").(string)
	nameParts := strings.Split(id, "/")
	if len(nameParts) != 4 { // you could be stricter about this check if you want.
	return nil, fmt.Errorf("env is expected to have shape organizations/{{"{{"}}org_id{{"}}"}}/environments/{{"{{"}}env{{"}}"}}, got %s instead", id)
	}
	d.SetId(id)

Any errors parsing the import id need to be returned - otherwise you might end up with an import that looks like it succeeds but fails to actually set an id (which means the resource won't actually be imported).