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

refactor: Refactoring to match the rest of terraform-aws-modules #1583

Merged
merged 12 commits into from
Sep 16, 2021

Conversation

antonbabenko
Copy link
Member

@antonbabenko antonbabenko commented Sep 13, 2021

This refactoring had an intention to keep all existing functionality and fix obvious issues:

  1. critical bugs (like syntax errors)
  2. remove unused variables and illogical parts of the code which were plain wrong
  3. add documentation for the missing parts (variables and outputs)
  4. update examples, so we can start trusting them again
  5. add tflint hook and make it happy (without breaking anything)

Backward compatibility is important here.

Fixes #1532
Fixes #1245

@antonbabenko antonbabenko marked this pull request as draft September 13, 2021 12:19
Copy link
Contributor

@daroga0002 daroga0002 left a comment

Choose a reason for hiding this comment

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

I know it is still draft, but I taken a fast look what is going on and left some comments which should be helpful

README.md Outdated Show resolved Hide resolved
examples/_bootstrap/main.tf Show resolved Hide resolved
examples/fargate/main.tf Outdated Show resolved Hide resolved
locals.tf Outdated Show resolved Hide resolved
locals.tf Outdated Show resolved Hide resolved
@antonbabenko
Copy link
Member Author

@daroga0002 Thanks for the initial review! Some of the comments are not relevant because I have not finished many of examples (WIP still). Let me ping you when I am ready for review - tomorrow or on Wednesday. I am changing a lot of files but trying to keep the same logic in all places.

@antonbabenko antonbabenko marked this pull request as ready for review September 14, 2021 15:20
@antonbabenko
Copy link
Member Author

@daroga0002 Please review this one.

The remaining bits in this PR are related to the examples (instance_refresh, irsa, launch_templates, launch_templates_with_managed_node_groups, managed_node_groups, secrets_encryption). I will look into that tomorrow morning.

@antonbabenko antonbabenko changed the title chore: Refactoring to match the rest of terraform-aws-modules refactor: Refactoring to match the rest of terraform-aws-modules Sep 14, 2021
Copy link
Contributor

@daroga0002 daroga0002 left a comment

Choose a reason for hiding this comment

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

I have mixed feelings about new example format where we need to execute a bootstrap (especially including a EKS there) because now when I will want to test soemthing then I must run bootstrap (15 minutes) and then second example (again 15 minutes).

Also dont like here that I must run one dir and then second (so I cannot just run it in background and return when it will be created).

One more point against it that potentially running few examples (I used to make such experiments) will create me kubernetes inside same VPC (created in bootstrap) what can cause some interference between clusters.

I am just loudly thinking but maybe better will be just exclude dependencies (VPC) into separate file vpc.tf which will be just copied into each example directory?

Beside this it looks good 👍

README.md Outdated Show resolved Hide resolved
Co-authored-by: Dawid Rogaczewski <[email protected]>
@antonbabenko
Copy link
Member Author

Thanks for the review and feedback!

I kind of agree that separation in examples is not very logical. At first, bootstrap didn't have EKS but just VPC which can be created once and it is free to keep running. Later, I worked on modules/fargate and respective examples for it which require the existing EKS cluster to put Fargate profiles into when the submodule is used separately.

I will change it now and move the EKS cluster from bootstrap into individual examples where it makes sense. This way we can run terraform apply just once in a single example folder we want to verify.

@antonbabenko antonbabenko merged commit 2bdf7d7 into master Sep 16, 2021
@antonbabenko antonbabenko deleted the chore-refactoring branch September 16, 2021 09:35
@antonbabenko
Copy link
Member Author

v17.19.0 has been just released.

If this release breaks something that was working before, let's fix it immediately. I did my best to not break anything and put breaking changes in the milestone v18.0.0.

Let's make sure that new PRs are rebased properly before we do a review.

@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 12, 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.

Unable to have more than one profile in fargate_profiles
2 participants