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: Update EKS module version and add additional variables supported by the module #1132

Merged
merged 2 commits into from
Nov 4, 2022

Conversation

bryantbiggs
Copy link
Contributor

What does this PR do?

  • Update EKS module version and add additional variables supported by the module
  • Add support for disabling cluster encryption
  • Remove workflow scripts that have since moved to a scripts/ directory

Motivation

  • Internal request

More

  • Yes, I have tested the PR using my local account setup (Provide any test evidence report under Additional Notes)
  • Yes, I have added a new example under examples to support my PR
  • Yes, I have created another PR for add-ons under add-ons repo (if applicable)
  • Yes, I have updated the docs for this feature
  • Yes, I ran pre-commit run -a with this PR

Note: Not all the PRs require a new example and/or doc page. In general:

  • Use an existing example when possible to demonstrate a new addons usage
  • A new docs page under docs/add-ons/* is required for new a new addon

For Moderators

  • E2E Test successfully complete before merge?

Additional Notes

@bryantbiggs bryantbiggs temporarily deployed to EKS Blueprints Test November 4, 2022 18:28 Inactive
@@ -1,35 +0,0 @@
import json
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@@ -1,30 +0,0 @@
import json
Copy link
Contributor Author

Choose a reason for hiding this comment

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

module "aws_eks" {
source = "terraform-aws-modules/eks/aws"
version = "v18.26.6"
version = "v18.29.1"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

we don't want to go up to v18.30 if we can avoid it - 18.30 added default tags logic that is proving to be problematic and will be removed/reverted in v19.x

Copy link
Contributor

@askulkarni2 askulkarni2 left a comment

Choose a reason for hiding this comment

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

🙌🏽

@bryantbiggs bryantbiggs merged commit 7382e06 into main Nov 4, 2022
@bryantbiggs bryantbiggs deleted the feat/eks-module-update branch November 4, 2022 18:51
# CLUSTER KMS KEY
# ---------------------------------------------------------------------------------------------------------------------
locals {
cluster_encryption_config = length(var.cluster_encryption_config) > 0 ? [
Copy link

@muram muram Nov 5, 2022

Choose a reason for hiding this comment

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

Hi @bryantbiggs , I believe this condition should evaluate if the cluster_encryption_config variable is not provided which means == 0 not > 0. The default value is an empty list [].

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah yes, you are correct. let me fix this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

PR #1137

Thank you for catching this @muram

vara-bonthu pushed a commit that referenced this pull request Nov 11, 2022
allamand pushed a commit to allamand/terraform-aws-eks-blueprints that referenced this pull request Dec 15, 2022
allamand pushed a commit to allamand/terraform-aws-eks-blueprints that referenced this pull request Jan 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants