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: Allow conditional creation of node groups to be set within node group definitions #1848

Merged
merged 2 commits into from
Feb 8, 2022

Conversation

PhilippMT
Copy link
Contributor

Description

  • Allow conditional creation of self_managed_node_group by passing the variable "create" to the module
  • If "create" is not present the default value "true" is used
  • If "create" is set to "false" the module is not created

Motivation and Context

We are using the same manifest for different stages. Some resources are created conditionally based on variables set for a specific stage. We defined multiple self managed node groups in the map self_managed_node_group, but not all of them are relevant for all stages. We would like to be able to create some of the entries conditionally on the "create" variable defined by the self_managed_node_group submodule.

Breaking Changes

  • No

How Has This Been Tested?

I tested three different scenarios

  1. create = true -> The self_managed_node_group was created with all its resources
  2. create = false -> The self_managed_node_group was ignored
  3. create missing -> The self_managed_node_group was create with all its resources

@PhilippMT
Copy link
Contributor Author

Sorry for my confusion, this is my first PR. As of the contributing guidelines https://github.com/terraform-aws-modules/terraform-aws-eks/blob/master/.github/CONTRIBUTING.md "improvement" is a valid PR title but the CI job does not allow this value. Should I alternatively use "feat"?

@@ -331,6 +331,8 @@ module "self_managed_node_group" {

for_each = { for k, v in var.self_managed_node_groups : k => v if var.create }

create = try(each.value.create, true)
Copy link
Member

Choose a reason for hiding this comment

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

I haven't tested this yet, but any changes we make like this should also be propagated to the EKS managed node group and Fargate profile definitions

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, I added the feature to the managed node groups and fargate profiles but did not test it, because I am not using them anywhere

@bryantbiggs
Copy link
Member

and regarding your title, yes lets just use feat: ... and it should pass the check then

@PhilippMT PhilippMT changed the title improvement: Allow conditional creation of self_managed_node_group feat: Allow conditional creation of self_managed_node_group Feb 7, 2022
@PhilippMT PhilippMT changed the title feat: Allow conditional creation of self_managed_node_group feat: Allow conditional creation of self_managed_node_group, eks_managed_node_group and fargate_profile Feb 7, 2022
Copy link
Member

@bryantbiggs bryantbiggs 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, one last change - lets change the title to feat: Allow conditional creation of node groups to be set within node group definitions - thanks for the PR @PhilippMT !

should be good to go with title change @antonbabenko 👍🏽

@PhilippMT PhilippMT changed the title feat: Allow conditional creation of self_managed_node_group, eks_managed_node_group and fargate_profile feat: Allow conditional creation of node groups to be set within node group definitions Feb 7, 2022
@antonbabenko antonbabenko merged commit 665f468 into terraform-aws-modules:master Feb 8, 2022
antonbabenko pushed a commit that referenced this pull request Feb 8, 2022
## [18.5.0](v18.4.1...v18.5.0) (2022-02-08)

### Features

* Allow conditional creation of node groups to be set within node group definitions ([#1848](#1848)) ([665f468](665f468))
@antonbabenko
Copy link
Member

This PR is included in version 18.5.0 🎉

baibailiha added a commit to baibailiha/terraform-aws-eks that referenced this pull request Sep 13, 2022
## [18.5.0](terraform-aws-modules/terraform-aws-eks@v18.4.1...v18.5.0) (2022-02-08)

### Features

* Allow conditional creation of node groups to be set within node group definitions ([#1848](terraform-aws-modules/terraform-aws-eks#1848)) ([581319f](terraform-aws-modules/terraform-aws-eks@581319f))
@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 Nov 10, 2022
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.

3 participants