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.

Sign up for GitHub

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

Jump to bottom

feat: Add create_custom_role_trust_policy to control when a custom_role_trust_policy should be used #321

Merged
merged 11 commits into from
Aug 31, 2023
Merged

feat: Add create_custom_role_trust_policy to control when a custom_role_trust_policy should be used #321

merged 11 commits into from
Aug 31, 2023

Conversation

egarbi
Copy link
Contributor

@egarbi egarbi commented Dec 19, 2022 •
edited
Loading

Description

Fixes #320

Motivation and Context

This is needed to prevent errors when trying to add a computed value for custom_role_trust_policy

Breaking Changes

Those already passing valid values into custom_role_trust_policy will need to
add an extra input create_custom_role_trust_policy = true to prevent Terraform
from trying to destroy policies.

How Has This Been Tested?

  • I have updated at least one of the examples/* to demonstrate and validate my change(s)
  • I have tested and validated these changes using one or more of the provided examples/* projects
  • I have executed pre-commit run -a on my pull request

@egarbi egarbi changed the title Workaround invalid count argument error fix: invalid count argument error when custom_role_trust_policy is computed Dec 19, 2022
@egarbi egarbi changed the title fix: invalid count argument error when custom_role_trust_policy is computed fix: Invalid count argument error when custom_role_trust_policy is computed Dec 19, 2022
@github-actions
Copy link

This PR has been automatically marked as stale because it has been open 30 days
with no activity. Remove stale label or comment or this PR will be closed in 10 days

@github-actions github-actions bot added the stale label Jan 19, 2023
@egarbi
Copy link
Contributor Author

egarbi commented Jan 19, 2023 •
edited
Loading

@antonbabenko can you please take a look at this?

antonbabenko
antonbabenko requested changes Jan 19, 2023
View reviewed changes
modules/iam-assumable-role/variables.tf Outdated Show resolved Hide resolved
modules/iam-assumable-role/main.tf Outdated Show resolved Hide resolved
@antonbabenko antonbabenko changed the title fix: Invalid count argument error when custom_role_trust_policy is computed feat: Added create_custom_role_trust_policy to control when custom_role_trust_policy should be created Jan 19, 2023
@github-actions github-actions bot removed the stale label Jan 20, 2023
egarbi added 2 commits January 20, 2023 11:20
@egarbi
fix(iam-assumable-role): Fix description variable
1b682e1
@egarbi
fix(iam-assumable-role): Prevent ignoring create_custom_role_trust_po…
1d9c0e4 
…licy

Fix an issue that allowed to created a trusted policy even when
create_custom_role_trust_policy was false.
A new local variable was added to grant the proper value based
on a condition
@egarbi egarbi force-pushed the workaround_invalid_count_argument_error branch from a13baaf to 1d9c0e4 Compare January 20, 2023 10:26
@egarbi egarbi requested a review from antonbabenko January 20, 2023 10:33
@github-actions
Copy link

This PR has been automatically marked as stale because it has been open 30 days
with no activity. Remove stale label or comment or this PR will be closed in 10 days

@github-actions github-actions bot added the stale label Feb 26, 2023
@egarbi
Copy link
Contributor Author

egarbi commented Feb 28, 2023

@antonbabenko would you mind checking again, please

@github-actions github-actions bot removed the stale label Mar 1, 2023
@schniber
Copy link

schniber commented Mar 4, 2023

Hey all,

@egarbi: I think you need to mark the changes that @antonbabenko requested as done in order to move forward with the process.

@bryantbiggs: any guidance on how to help get this merged ?

Thanks a lot !

Bests.

@schniber schniber mentioned this pull request Mar 31, 2023
Closed
3 tasks
@github-actions
Copy link

github-actions bot commented Apr 3, 2023

This PR has been automatically marked as stale because it has been open 30 days
with no activity. Remove stale label or comment or this PR will be closed in 10 days

@github-actions github-actions bot added the stale label Apr 3, 2023
@github-actions
Copy link

This PR was automatically closed because of stale in 10 days

@github-actions github-actions bot closed this Apr 14, 2023
@github-actions
Copy link

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 14, 2023
@bryantbiggs bryantbiggs reopened this Aug 31, 2023
@terraform-aws-modules terraform-aws-modules unlocked this conversation Aug 31, 2023
@bryantbiggs bryantbiggs changed the title feat: Added create_custom_role_trust_policy to control when custom_role_trust_policy should be created feat: Add create_custom_role_trust_policy to control when a custom_role_trust_policy should be used Aug 31, 2023
@bryantbiggs
Copy link
Member

@antonbabenko ok from your end?

@antonbabenko antonbabenko merged commit 481095e into terraform-aws-modules:master Aug 31, 2023
35 checks passed
antonbabenko pushed a commit that referenced this pull request Aug 31, 2023
@semantic-release-bot
chore(release): version 5.30.0 [skip ci]
856b543 
## [5.30.0](v5.29.2...v5.30.0) (2023-08-31)

### Features

* Add `create_custom_role_trust_policy` to control when a `custom_role_trust_policy` should be used ([#321](#321)) ([481095e](481095e))
@antonbabenko
Copy link
Member

This PR is included in version 5.30.0 🎉

@vladpanainte vladpanainte mentioned this pull request Sep 1, 2023
Closed
schniber added a commit to schniber/terraform-aws-iam that referenced this pull request Sep 1, 2023
@schniber
resolve conflicts with terraform-aws-modules#321
3775c37
schniber added a commit to schniber/terraform-aws-iam that referenced this pull request Sep 1, 2023
@schniber
fix conflicts with terraform-aws-modules#321
931b41f
@fregatte
Copy link

fregatte commented Sep 6, 2023

Hi mates, small note here from my side.
This is a breaking change because we are required to make changes in your code to keep the behavior the same as the previous version.
Let me explain. I have configured custom_role_trust_policy with the 5.29.2 module and by default, it was created on apply. On the 5.30.0 version default behavior was changed to create=false and to have the same behavior I need to add the create_custom_role_trust_policy = true explicitly. It's not a minor change in my opinion and it can break something in case you don't use strict constraints, e.g. ~>5.0.
So, #420 above is an example of such an issue.
Do you agree or do I have a lack of understanding of modules versioning?

@egarbi
Copy link
Contributor Author

egarbi commented Sep 6, 2023

Hi mates, small note here from my side. This is a breaking change because we are required to make changes in your code to keep the behavior the same as the previous version. Let me explain. I have configured custom_role_trust_policy with the 5.29.2 module and by default, it was created on apply. On the 5.30.0 version default behavior was changed to create=false and to have the same behavior I need to add the create_custom_role_trust_policy = true explicitly. It's not a minor change in my opinion and it can break something in case you don't use strict constraints, e.g. ~>5.0. So, #420 above is an example of such an issue. Do you agree or do I have a lack of understanding of modules versioning?

In my defence, I will say that the breaking change was mentioned in the description.

@fregatte
Copy link

fregatte commented Sep 6, 2023

In my defence, I will say that the breaking change was mentioned in the description.

I'm not trying to blame so you no need to defend :)
Just highlighted that IMO it would be more appropriate to bump the major version of the module instead of the minor. Just to clarify my understanding and make a note for future releases.

@egarbi
Copy link
Contributor Author

egarbi commented Sep 6, 2023

In my defence, I will say that the breaking change was mentioned in the description.

I'm not trying to blame so you no need to defend :) Just highlighted that IMO it would be more appropriate to bump the major version of the module instead of the minor. Just to clarify my understanding and make a note for future releases.

I agree

@github-actions
Copy link

github-actions bot commented Oct 7, 2023

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 7, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@bryantbiggs bryantbiggs bryantbiggs approved these changes

@antonbabenko antonbabenko antonbabenko approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Module "iam-assumable-role" Invalid count value when policy is computed
5 participants
@egarbi @schniber @bryantbiggs @antonbabenko @fregatte