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(aws_instance): conditional IAM instance profile #182

Merged

Conversation

haidargit
Copy link
Contributor

@haidargit haidargit commented Jan 21, 2024

what

This PR enables a conditional iam_instance_profile variable for the ec2 module.

why

This feature request offers users the flexibility to decide whether to associate an IAM instance profile with their instances.

references

Kindly review this PR for module improvements. Thank you

@haidargit haidargit requested review from a team as code owners January 21, 2024 08:29
@haidargit
Copy link
Contributor Author

Hi @kevcube , @nitrocode
would you help to review the PR or any other changes that need to be added?

Thank you

@nitrocode
Copy link
Member

Did you try setting the instance_profile to an empty string to disable its creation without an additional flag?

@haidargit
Copy link
Contributor Author

which additional flag do you mean, @nitrocode ?
could you help comment or point to the code/PR?

Thanks 🙌

@joe-niland
Copy link
Member

@nitrocode instance_profile == "" signifies that the default instance profile and role should be created, whereas #180 is requesting that it be possible to create an instance without a profile + role.

@joe-niland joe-niland self-requested a review January 22, 2024 02:40
@haidargit
Copy link
Contributor Author

Thank you, yes that's what I meant for this feature request. @joe-niland , @nitrocode

@joe-niland
Copy link
Member

/terratest

@joe-niland joe-niland added the minor New features that do not break anything label Jan 22, 2024
@joe-niland
Copy link
Member

Hi @haidargit please have a look at the Terratest failures: https://github.com/cloudposse/actions/actions/runs/7605926145/job/20710921759

@haidargit
Copy link
Contributor Author

okay @joe-niland, i am checking on it

@haidargit
Copy link
Contributor Author

i have committed the changes for applying the consistent types for the true and false result expressions to these two outputs

output "role" & output "role_arn"

could you help test again @joe-niland ?
Thanks!

@joe-niland
Copy link
Member

/terratest

@joe-niland
Copy link
Member

@haidargit all seems fine. I will just make sure @nitrocode is OK with the PR as well before we proceed.

@nitrocode
Copy link
Member

I defer to you folks. Thanks for including me. :)

@joe-niland joe-niland merged commit bf54345 into cloudposse:main Jan 24, 2024
11 checks passed
@joe-niland
Copy link
Member

Cheers @nitrocode!

Thanks for your contribution @haidargit !

@haidargit
Copy link
Contributor Author

your welcome @joe-niland, @joe-niland 👍🏻 👍🏻

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
minor New features that do not break anything
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants