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 addons to latest supported versions #1096

Merged
merged 2 commits into from
Oct 28, 2022
Merged

Conversation

bryantbiggs
Copy link
Contributor

@bryantbiggs bryantbiggs commented Oct 27, 2022

What does this PR do?

  • Update addons to latest supported versions
  • Update tflint to current version and correct linting warnings (mostly around use of *)
  • Remove unnecessary provider requirements

Motivation

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 October 27, 2022 16:22 Inactive
global:
grafana:
enabled: false
proxy: false

imageVersion: prod-1.96.0
imageVersion: prod-1.97.0

Choose a reason for hiding this comment

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

Can you take the hardcoded versions out of the values.yaml file and have it managed by the helm version like the other yaml files? That way we can easily upgrade

Copy link
Contributor

@Zvikan Zvikan left a comment

Choose a reason for hiding this comment

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

LGTM, minor comments and question, also, let's make sure to run some e2e for sanity check.
Thank you!

namespace = local.namespace
timeout = 1200
Copy link
Contributor

Choose a reason for hiding this comment

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

My brain is telling me there's a reason why we set this timeout to be longer than the default, @askulkarni2 may know better

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the helm-addon already defaults to this so its redundant here

timeout = try(var.helm_config["timeout"], 1200)

namespace = "kube-system"
timeout = "1200"
Copy link
Contributor

Choose a reason for hiding this comment

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

same as argo, I want to say there's a reason why we increased the timeout for specific addons, but I also remember there were times where the addons were trying to be deployed before nodes were even provisioned.
we need to keep an eye to see if we'll get reports of failed apply's due to addons timeouts (argo/albc)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

default already for helm-addon

timeout = try(var.helm_config["timeout"], 1200)

chart = local.name
repository = "https://aws.github.io/eks-charts"
version = "0.0.3"
namespace = kubernetes_namespace_v1.csi_secrets_store_provider_aws.metadata[0].name
Copy link
Contributor

Choose a reason for hiding this comment

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

good change using implicit dependency instead of using depends_on

@@ -1,4 +1,4 @@
output "argocd_gitops_config" {
description = "Configuration used for managing the add-on with ArgoCD"
value = var.manage_via_gitops ? local.argocd_gitops_config : null
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't we want to show all the other gitops config there are?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

You are right, I read this wrong, I expected to see more dynamic configuration but it's local that mostly have enable=true and in some cases the service account name.

@bryantbiggs
Copy link
Contributor Author

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.

Update addon versions to be current
3 participants