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

fix: Add missing locals in iam-assumable-role module #290

Merged
merged 11 commits into from
Nov 1, 2022
Merged

fix: Add missing locals in iam-assumable-role module #290

merged 11 commits into from
Nov 1, 2022

Conversation

enver
Copy link
Contributor

@enver enver commented Oct 16, 2022 •
edited

Description

Fixes #289

Motivation and Context

Fixes #289

Breaking Changes

No

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

@enver enver marked this pull request as draft October 16, 2022 08:08
@enver enver changed the title Add missing locals in iam-assumable-role module fix: Add missing locals in iam-assumable-role module Oct 16, 2022
@sirantd
Copy link
Contributor

sirantd commented Oct 16, 2022

Please take a look at line No. 81 of the main.tf, I believe we need to keep consistency and either use local. in both places or not use them in both places.

@enver enver marked this pull request as ready for review October 16, 2022 16:29
@enver
Copy link
Contributor Author

enver commented Oct 16, 2022 •
edited

@sirantd I don't understand your comment. Could you please add more details?

@sirantd
Copy link
Contributor

sirantd commented Oct 16, 2022 •
edited

@enver It has been fixed in your second commit, all is good now in the file

image

but the same problem here and in other files
image

@LucaIcaro
Copy link

Can this be approved? We have some workflows that are blocked because of this issue.

@sirantd
Copy link
Contributor

sirantd commented Oct 24, 2022

I made PR to the @enver branch. Once merged - it looks ok to me.

@enver
Merge pull request #1 from sirantd/fix-missing-locals
f2d633b 
Replace missed data reference to local variable for consistency
@enver
Copy link
Contributor Author

enver commented Oct 24, 2022

@sirantd Sorry, I just couldn't find time to finish this. Thank you for jumping in...merged.

@LucaIcaro
Copy link

Looks like there is still the need of a further approval

@sirantd
Copy link
Contributor

sirantd commented Oct 26, 2022

Hi @bryantbiggs, does it makes sense to do fixes to pass linter warnings for terraform 1.3.3 as part of this PR? It's already quite big.

Could you review it so it can be merged, please?

@bryantbiggs bryantbiggs mentioned this pull request Oct 28, 2022
Closed
3 tasks
@tegail
Copy link

tegail commented Oct 29, 2022

can we get someone to review this? it is also blocking workflows for us as well

@saurabh-paystack
Copy link

When can we expect this to be merged, please?

antonbabenko
antonbabenko requested changes Oct 31, 2022
View reviewed changes
Copy link
Member

@antonbabenko antonbabenko left a comment

Choose a reason for hiding this comment

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

Minor fixes and, most importantly, all examples should be working.

examples/iam-assumable-roles-with-saml/main.tf Show resolved Hide resolved
modules/iam-assumable-role-with-oidc/main.tf Show resolved Hide resolved
modules/iam-assumable-role-with-oidc/main.tf Outdated Show resolved Hide resolved
antonbabenko
antonbabenko approved these changes Nov 1, 2022
View reviewed changes
Copy link
Member

@antonbabenko antonbabenko left a comment

Choose a reason for hiding this comment

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

Looks good. As said, examples/iam-eks-role is failing, but it is not related to this PR.

@antonbabenko antonbabenko merged commit 8af6d28 into terraform-aws-modules:master Nov 1, 2022
antonbabenko pushed a commit that referenced this pull request Nov 1, 2022
@semantic-release-bot
chore(release): version 5.5.5 [skip ci]
37f2b5e 
### [5.5.5](v5.5.4...v5.5.5) (2022-11-01)

### Bug Fixes

* Add missing locals in iam-assumable-role module ([#290](#290)) ([8af6d28](8af6d28))
@antonbabenko
Copy link
Member

This PR is included in version 5.5.5 🎉

@enver enver deleted the fix-missing-locals branch November 2, 2022 07:57
@github-actions
Copy link

github-actions bot commented Dec 3, 2022

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 Dec 3, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@sirantd sirantd sirantd left review comments

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

Missing partition and account_id locals in multiple sub-modules in 5.5.2
8 participants
@enver @sirantd @LucaIcaro @tegail @saurabh-paystack @antonbabenko @bryantbiggs @enver-multibank