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 support for restoring default organization policies #1477

Merged
merged 7 commits into from
May 30, 2018

Conversation

ortaman
Copy link
Contributor

@ortaman ortaman commented May 11, 2018

To the enhancement: #1239

Copy link
Contributor

@emilymye emilymye left a comment

Choose a reason for hiding this comment

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

Does this get read in after a change? i.e. after I change my config to restore_default and run terraform apply to edit the org policy, does running terraform plan show changes?

}

if restore_default != nil {

Copy link
Contributor

Choose a reason for hiding this comment

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

nit - remove line

Copy link
Contributor Author

@ortaman ortaman May 15, 2018

Choose a reason for hiding this comment

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

refactored code

@@ -32,7 +32,8 @@ var schemaOrganizationPolicy = map[string]*schema.Schema{
Type: schema.TypeList,
Optional: true,
MaxItems: 1,
ConflictsWith: []string{"boolean_policy"},
ConflictsWith: []string{"boolean_policy", "restore_policy"},

Copy link
Contributor

Choose a reason for hiding this comment

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

-newline

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

}).Do()

} else {

Copy link
Contributor

Choose a reason for hiding this comment

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

-newline

Copy link
Contributor Author

Choose a reason for hiding this comment

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

refactored code

RestoreDefault: restore_default,
},
}).Do()

Copy link
Contributor

Choose a reason for hiding this comment

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

-newline

Copy link
Contributor Author

Choose a reason for hiding this comment

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

refactored code

},
}).Do()
if restore_default != nil {

Copy link
Contributor

Choose a reason for hiding this comment

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

-newline

Copy link
Contributor Author

Choose a reason for hiding this comment

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

refactored code

}).Do()

} else {

Copy link
Contributor

Choose a reason for hiding this comment

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

-newline

Copy link
Contributor Author

@ortaman ortaman May 15, 2018

Choose a reason for hiding this comment

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

refactored code

}

if restore_default != nil {

Copy link
Contributor

Choose a reason for hiding this comment

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

-newline

Copy link
Contributor Author

@ortaman ortaman May 15, 2018

Choose a reason for hiding this comment

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

refactored code

RestoreDefault: restore_default,
},
}).Do()

Copy link
Contributor

Choose a reason for hiding this comment

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

-newline

Copy link
Contributor Author

Choose a reason for hiding this comment

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

code refactored

@ortaman ortaman force-pushed the master branch 2 times, most recently from 1afcaed to 739251a Compare May 15, 2018 01:00
@ortaman
Copy link
Contributor Author

ortaman commented May 15, 2018

Hi @emilymye, I updated the PR with some refactored code, in another hand is not showing changes if the example is ok.

[root@localhost dev-org-policy]# terraform apply
google_organization_policy.services_policy: Refreshing state... (ID: 65779779009:serviceuser.services)
google_organization_policy.services_policy: Modifying... (ID: 65779779009:serviceuser.services)
list_policy.#: "1" => "0"
list_policy.0.allow.#: "1" => "0"
list_policy.0.allow.0.all: "true" => "false"
restore_policy.#: "0" => "1"
restore_policy.0.default: "" => "true"
google_organization_policy.services_policy: Modifications complete after 1s (ID: 65779779009:serviceuser.services)

Apply complete! Resources: 0 added, 1 changed, 0 destroyed.
[root@localhost dev-org-policy]# terraform plan
Refreshing Terraform state in-memory prior to plan...
The refreshed state will be used to calculate this plan, but will not be
persisted to local or remote state storage.

google_organization_policy.services_policy: Refreshing state... (ID: 65779779009:serviceuser.services)


No changes. Infrastructure is up-to-date.

This means that Terraform did not detect any differences between your
configuration and real physical resources that exist. As a result, no
actions need to be performed.
[root@localhost dev-org-policy]#

Copy link
Contributor

@emilymye emilymye left a comment

Choose a reason for hiding this comment

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

LGTM, assigning to @danawillow to check tests for orgs

@emilymye emilymye requested a review from danawillow May 23, 2018 19:49
listPolicy, err := expandListOrganizationPolicy(d.Get("list_policy").([]interface{}))
if err != nil {
return err
}

restore_default, err := expandRestoreOrganizationPolicy(d.Get("restore_policy").([]interface{}))
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: please camelCase your variable names

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done


resource "google_folder_organization_policy" "restore" {
folder = "${google_folder.orgpolicy.name}"
constraint = "serviceuser.services"
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: some of these lines use tabs, others use spaces

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed to use only spaces

@@ -227,6 +248,20 @@ func flattenBooleanOrganizationPolicy(policy *cloudresourcemanager.BooleanPolicy
return bPolicies
}

func flattenRestoreOrganizationPolicy(restore_policy *cloudresourcemanager.RestoreDefault) []map[string]interface{} {
bRestore_policy := make([]map[string]interface{}, 0, 1)
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure what the b prefix means here. Maybe just call it rp or something simple like that?

Copy link
Contributor Author

@ortaman ortaman May 23, 2018

Choose a reason for hiding this comment

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

changed to rp (prefix doesnt mean anything)

return nil, nil
}

if len(configured) > 0 {
Copy link
Contributor

Choose a reason for hiding this comment

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

This if statement isn't necessary since we already checked above if it's equal to 0

Copy link
Contributor Author

Choose a reason for hiding this comment

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

because there are three escenarios there:
1.- when we doesnt have the restore_policy argument.
2.-when we have the restore_policy argument, but the default value is not true.
3.-when we have the restore_policy argument and the default value is true.

Copy link
Contributor

Choose a reason for hiding this comment

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

Right, it could look like this:

	if len(configured) == 0 {
		return nil, nil
	}

	restoreDefaultMap := configured[0].(map[string]interface{})
	default_value := restoreDefaultMap["default"].(bool)
 
	if default_value {
		return &cloudresourcemanager.RestoreDefault{}, nil
	}

	return nil, fmt.Errorf("Invalid value for restore_policy. Expecting default = true")

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, one 'if' is not necessary and looks cleaner, good catch (changed).

Schema: map[string]*schema.Schema{
"default": {
Type: schema.TypeBool,
Required: true,
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm a bit confused about the structure of this. The API requires an empty object, so default is just a construct here- what made you decide to go for a nested object with a boolean that has to be set to true, instead of just top-leveling that boolean (i.e. restore_default = true)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

only because in the requirements give me the next structure:
restore_policy {
default = true
}

Copy link
Contributor

Choose a reason for hiding this comment

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

@morgante, this was your idea I believe. What do you think?

Copy link

@morgante morgante May 24, 2018

Choose a reason for hiding this comment

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

The reason I specified it that way was for equivalence with the other constraint types, which use a nested structure. boolean_policy, for example, also only has a single boolean value but is also nested. Does that make sense?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, it makes sense. It's a little different in this case because the others are mirroring the API exactly as it is, whereas this one is adding a field into an object, even though that field doesn't exist. I do think it's probably less confusing than just requiring an empty object though, and a bit more future proof anyway, so sure. I'm down.

@danawillow
Copy link
Contributor

Also as an fyi, if in your commit message you say "fixes #___" then it'll automatically close the issue once the PR is merged :)

@ortaman ortaman force-pushed the master branch 2 times, most recently from 8fb6670 to 484d6dd Compare May 23, 2018 23:47
ortaman pushed a commit to ortaman/terraform-provider-google that referenced this pull request May 23, 2018
ortaman pushed a commit to ortaman/terraform-provider-google that referenced this pull request May 23, 2018
ortaman pushed a commit to ortaman/terraform-provider-google that referenced this pull request May 24, 2018
return &cloudresourcemanager.RestoreDefault{}, nil
}

return nil, fmt.Errorf("Invalid value for restore_policy. Expecting default = true")
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of this, what do you think about adding a ValidateFunc to the field? That way, if someone sets the default to false, it would get caught at plan-time instead of apply-time.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added ValidateFunc

project = "%s"
constraint = "constraints/serviceuser.services"

restore_policy {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: this is indented a bit too far

Copy link
Contributor Author

Choose a reason for hiding this comment

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

changed

@@ -96,6 +112,10 @@ The `allow` or `deny` blocks support:

* `values` - (Optional) The policy can define specific values that are allowed or denied.

The `restore_policy` block supports:

* `default` - (Required) If true, then the default Policy is restored. If false, then any configuration is acceptable.
Copy link
Contributor

Choose a reason for hiding this comment

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

Actually, if it's set to false then it throws an error.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

changed

listPolicy, err := expandListOrganizationPolicy(d.Get("list_policy").([]interface{}))
if err != nil {
return err
}

restore_default, err := expandRestoreOrganizationPolicy(d.Get("restore_policy").([]interface{}))
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: This should be camelcase

Copy link
Contributor Author

Choose a reason for hiding this comment

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

changed

}

restoreDefaultMap := configured[0].(map[string]interface{})
default_value := restoreDefaultMap["default"].(bool)
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: camelcase

Copy link
Contributor Author

Choose a reason for hiding this comment

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

changed

@@ -227,6 +248,20 @@ func flattenBooleanOrganizationPolicy(policy *cloudresourcemanager.BooleanPolicy
return bPolicies
}

func flattenRestoreOrganizationPolicy(restore_policy *cloudresourcemanager.RestoreDefault) []map[string]interface{} {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: camelcase

Copy link
Contributor Author

Choose a reason for hiding this comment

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

changed

@@ -249,7 +283,7 @@ resource "google_folder" "orgpolicy" {
}

resource "google_folder_organization_policy" "bool" {
# Test numeric folder ID.
# Test numeric folder ID.
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: can you revert the extra indents here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@ortaman ortaman force-pushed the master branch 3 times, most recently from b9f3b97 to deeaac5 Compare May 30, 2018 22:28
Copy link
Contributor

@danawillow danawillow left a comment

Choose a reason for hiding this comment

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

Thanks @ortaman! Looks good!

@danawillow danawillow merged commit 8a77e42 into hashicorp:master May 30, 2018
chrisst pushed a commit to chrisst/terraform-provider-google that referenced this pull request Nov 9, 2018
…1477)

* Added support for restoring default organization policies

* Added support for restoring default folder/project organization policies

* Refactored code

* Fixes hashicorp#1239

* Clarify docs

* Clarify docs

* Clarify docs
@ghost
Copy link

ghost commented Nov 18, 2018

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. If you feel I made an error 🤖 🙉 , please reach out to my human friends 👉 [email protected]. Thanks!

@ghost ghost locked and limited conversation to collaborators Nov 18, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants