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

feat: define iam_policy_statements object syntax #26

Merged
merged 8 commits into from Jun 10, 2023

Conversation

gberenice
Copy link
Contributor

@gberenice gberenice commented Jun 8, 2023

what

  • This updates iam_policy_statements variable definition to take Terraform object with optional attributes rather than type = any. Provider version is bumped since optional, as a non-experimental feature, was introduced in 1.3.

why

  • Provide more control over the expected structure of the input data for IAM policy statements.

references

@gberenice gberenice requested review from a team as code owners June 8, 2023 14:53
@gberenice gberenice force-pushed the feature/support_optional_attributes branch from 8960d44 to f9191c9 Compare June 8, 2023 15:31
@Gowiem
Copy link
Member

Gowiem commented Jun 8, 2023

/terratest

@Gowiem
Copy link
Member

Gowiem commented Jun 8, 2023

/terratest

Nuru
Nuru previously requested changes Jun 8, 2023
variables.tf Outdated
type = string
identifiers = list(string)
})), [])
})
description = "Map of IAM policy statements to use in the policy. This can be used with or instead of the `var.iam_source_json_url`."
Copy link
Sponsor Contributor

Choose a reason for hiding this comment

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

Change "Map of" to "Object describing"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Change "Map of" to "Object describing"

@Nuru from what I can see - a map is expected here, so I fixed the variable definition.

variables.tf Outdated
Comment on lines 8 to 14
type = object({
sid = optional(string, "")
effect = optional(string, "")
actions = optional(list(string), [])
not_actions = optional(list(string), [])
resources = optional(list(string), [])
not_resources = optional(list(string), [])
Copy link
Sponsor Contributor

Choose a reason for hiding this comment

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

  • Add version here and in main.tf.
  • Move statement components to list of objects.
  • Inputs that are not going to be expanded into blocks by dynamic should default to null.
  • In main.tf, replace lookups like
lookup(statement.value, "sid", statement.key)

With direct references

statement.value.sid
Suggested change
type = object({
sid = optional(string, "")
effect = optional(string, "")
actions = optional(list(string), [])
not_actions = optional(list(string), [])
resources = optional(list(string), [])
not_resources = optional(list(string), [])
type = object({
version = optional(string, null)
statement = list(object({
sid = optional(string, null)
effect = optional(string, null)
actions = optional(list(string), null)
not_actions = optional(list(string), null)
resources = optional(list(string), null)
not_resources = optional(list(string), null)
})), [])

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Nuru thanks a lot for your review!

  • Version arguments should be set for data source aws_iam_policy_document, so I added it as a separate variable.
  • I updated statements to the map of objects - that's what is expected in the example.
  • Inputs were updated.
  • Lookups were replaced👍

Please let me know your thoughts.

Copy link
Sponsor Contributor

Choose a reason for hiding this comment

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

@gberenice I forgot we want this to be backward compatible and that means the input is a map. I will take it from here. Thank you very much for your contributions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Nuru sounds good, thank you!

@gberenice gberenice force-pushed the feature/support_optional_attributes branch from 36cf1ff to 43364f4 Compare June 9, 2023 12:31
@Nuru
Copy link
Sponsor Contributor

Nuru commented Jun 10, 2023

/terratest

@Nuru Nuru dismissed their stale review June 10, 2023 00:34

Changes made

@Nuru Nuru added the major Breaking changes (or first stable release) label Jun 10, 2023
variables.tf Outdated Show resolved Hide resolved
variables.tf Outdated Show resolved Hide resolved
Copy link
Member

@aknysh aknysh left a comment

Choose a reason for hiding this comment

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

please see comments

@Nuru Nuru requested a review from aknysh June 10, 2023 05:38
@Nuru
Copy link
Sponsor Contributor

Nuru commented Jun 10, 2023

/terratest

@@ -8,8 +8,68 @@ variable "iam_source_json_url" {
default = null
}

Copy link
Member

Choose a reason for hiding this comment

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

please update the description for the iam_source_json_url variable in the example as well

description = <<-EOT
    URL of the IAM policy (in JSON format) to download and use as `source_json` argument.
    This is useful when using a 3rd party service that provides their own policy.
    This can be used with or instead of `var.iam_policy`.
    EOT

Copy link
Member

@aknysh aknysh left a comment

Choose a reason for hiding this comment

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

please see comment

@Nuru Nuru enabled auto-merge (squash) June 10, 2023 20:41
@Nuru
Copy link
Sponsor Contributor

Nuru commented Jun 10, 2023

/terratest

@Nuru Nuru requested a review from aknysh June 10, 2023 20:41
@Nuru Nuru merged commit 660de2d into cloudposse:main Jun 10, 2023
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
major Breaking changes (or first stable release)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants