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 readme with design considerations. #649

Closed

Conversation

TBeijen
Copy link

@TBeijen TBeijen commented Dec 27, 2019

PR o'clock

Description

Draft 'Readme-driven-development' PR, addressing #635 (specifically this comment) and feedback in #644.

Feedback welcome. Will add code soon.

Checklist

@dpiddockcmp
Copy link
Contributor

I was fiddling around with trying to move this code into a submodule. Current progress has been put in PR #650.

I assumed that the submodule would only be used by the parent module as a way to isolate code. No way to include a module multiple times at runtime yet, so all appropriate multiple resources are created inside the module.

I have concerns that there would be too much specific plumbing required to have the module used stand alone by the user.

@TBeijen
Copy link
Author

TBeijen commented Dec 29, 2019

I have concerns that there would be too much specific plumbing required to have the module used stand alone by the user.

I'll take a look (probably after the weekend). Imo there's various angles to look at:

  • Complexity of module code (maintenance, users looking into module code for usage details)
  • Need for users to wire outputs into submodule inputs.
  • Implicit pros/cons, e.g. the current inability to remove the first worker group from a list of worker groups.

I might code a poc to just see how much the plumbing will be.

Having the user repeat a module blocks feels natural. It's using Terraform's concept for re-usable code (modules) instead of re-creating that ourselves within the terraform-aws-eks module. It's wat Cloudposse does.

As for plumbing, the idea crossed my mind to have the parent module (terraform-aws-eks core) output all relevant attributes separately but also wrapped in a map, a sort of 'connector'. Then wire that into the submodule. Something like:

module "my_cluster" {
  source          = "terraform-aws-modules/eks/aws"
  cluster_name    = "my-cluster"
}

module "managed_group_a" {
  source          = "terraform-aws-modules/eks/aws//modules/managed_nodes"
  node_group_name = "managed-a"

  parent_module_attributes = module.my_cluster.attributes_combined
}

Naming not so good. And might very well best remain an idea.

@stale
Copy link

stale bot commented Mar 28, 2020

This PR has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.
To track this PR (even if closed), please open a corresponding issue if one does not already exist.

@stale stale bot added the stale label Mar 28, 2020
@barryib
Copy link
Member

barryib commented Apr 22, 2020

@TBeijen this is still in draft and sounds like this it goes stale.

Closing this. Please feel free to reopen when work resumes.

@barryib barryib closed this Apr 22, 2020
@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 18, 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