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

Added timeout configs and variables to aws_eks_cluster resource #149

Conversation

RGPosadas
Copy link
Contributor

@RGPosadas RGPosadas commented Oct 2, 2018

PR o'clock

Description

Added timeout configs to the aws_eks_cluster resource.

Test Result

I set the timeouts to 1m to test for failure (could not test timeout for more than default time since it is unpredictable when it would happen). Here are the results:

1 error(s) occurred:
TestEKS 2018-10-02T12:32:30-04:00 command.go:100: 
TestEKS 2018-10-02T12:32:30-04:00 command.go:100: * module.test_eks_module.module.eks.aws_eks_cluster.this: 1 error(s) occurred:
TestEKS 2018-10-02T12:32:30-04:00 command.go:100: 
TestEKS 2018-10-02T12:32:30-04:00 command.go:100: * aws_eks_cluster.this: timeout while waiting for state to become 'ACTIVE' (last state: 'CREATING', timeout: 1m0s)

README.md Changes

I tried to update README.md using terraform-docs and the given lines of code, but that ended up erasing the whole file for me. So I added in the new variables manually.

Checklist

  • terraform fmt and terraform validate both work from the root and examples/eks_test_fixture directories (look in CI for an example)
  • Tests for the changes have been added and passing (for bug fixes/features)
  • Test results are pasted in this PR (in lieu of CI)
  • Docs have been updated using terraform-docs per README.md instructions
  • I've added my change to CHANGELOG.md
  • Any breaking changes are highlighted above

@laverya
Copy link
Contributor

laverya commented Oct 2, 2018

I think it might be best if you merge master into this branch before going much farther - it looks like your branch is off of b6f6a82, and there have been a number of changelog additions since then. (which is causing the merge conflict)

I like the change though! I've certainly run into the create timeout before.

Re: README.md, what version of terraform-docs (and go, and the os...) are you running? I just tried generating the docs again with this set of commands:

go get -u github.com/segmentio/terraform-docs
terraform-docs md ./ | cat -s | tail -r | tail -n +2 | tail -r > README.md

and had no differences.

Copy link
Contributor

@mmcaya mmcaya left a comment

Choose a reason for hiding this comment

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

👍 to having this capability, small note on variable naming inline

variables.tf Outdated
@@ -123,3 +123,13 @@ variable "kubeconfig_name" {
description = "Override the default name used for items kubeconfig."
default = ""
}

variable "aws_eks_cluster_create_timeout" {
Copy link
Contributor

Choose a reason for hiding this comment

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

For consistency with other cluster_* related variables and readability (e.g. cluster_version, should these be named cluster_create_timeout and cluster_delete_timeout?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, I'll change it

@RGPosadas
Copy link
Contributor Author

Hey @laverya !
I synced up my fork with master and resolved the merge conflict.

@mmcaya
Copy link
Contributor

mmcaya commented Oct 3, 2018

Thanks @RGPosadas, README.md should also be updated to reflect the new var names.

@RGPosadas
Copy link
Contributor Author

RGPosadas commented Oct 3, 2018

I updated README.md!

Also re: README.md, had to change tail -r to tac in

go get -u github.com/segmentio/terraform-docs
terraform-docs md ./ | cat -s | tac | tail -n +2 | tac > README.md

and it worked.

@max-rocket-internet
Copy link
Contributor

Thanks for your first contribution @RGPosadas!

I just fixed the travis error in master, could you rebase so we can get it to pass? Thanks!

@RGPosadas
Copy link
Contributor Author

@max-rocket-internet done!
And it was a pleasure to contribute! 😄

@max-rocket-internet max-rocket-internet merged commit 0ee9d63 into terraform-aws-modules:master Oct 4, 2018
nicgrayson pushed a commit to nicgrayson/terraform-aws-eks that referenced this pull request Oct 29, 2018
* upstream/master: (25 commits)
  Update documentation for removed `configure_kubectl_session` (terraform-aws-modules#171)
  remove the checksum step
  Add target_group_arns to worker ASG (terraform-aws-modules#167)
  Removing 2 providers from the module (terraform-aws-modules#168)
  Removing aws_iam_service_linked_role from module (terraform-aws-modules#160)
  Adjust the order and correct/update the info (terraform-aws-modules#163)
  Ruby ver `2.4.2` -> `2.4.4`
  Move env vars into env section
  Remove `v` in `v0.11.8`
  Better version control
  Add suspended_processes attributes to autoscaling_group (terraform-aws-modules#159)
  Updating changelog for v1.7.0 (terraform-aws-modules#158)
  Revert "Add suspended_processes attributes to autoscaling_group (terraform-aws-modules#153)" (terraform-aws-modules#157)
  Add suspended_processes attributes to autoscaling_group (terraform-aws-modules#153)
  Add option to change worker placement_tenancy. (terraform-aws-modules#142)
  Allowing 443 to nodes from EKS service (terraform-aws-modules#148)
  Fixed issue with 'workers_group_defaults_defaults.iam_role_id' and added explicit depends_on for 'update_config_map_aws_auth' (terraform-aws-modules#147)
  Added timeout configs and variables to aws_eks_cluster resource (terraform-aws-modules#149)
  Fixing travis config (terraform-aws-modules#151)
  Fix for ERROR: 'aws_iam_instance_profile.workers' not found (terraform-aws-modules#141)
  ...
aykamko pushed a commit to crowd-ai/terraform-aws-eks that referenced this pull request Dec 14, 2018
…aform-aws-modules#149)

* Added timeout configs and variables

* Updated CHANGELOG and README

* Added timeout configs and variables

* Updated CHANGELOG and README

* Changed variable names for consistency

* Updated README.md

* Did terraform fmt
@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 24, 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.

4 participants