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

Set up submodule structure #61

Merged
merged 12 commits into from
Jan 10, 2019
Merged

Set up submodule structure #61

merged 12 commits into from
Jan 10, 2019

Conversation

Jberlinsky
Copy link
Contributor

@Jberlinsky Jberlinsky commented Jan 9, 2019

Per conversation in #51 (review) and offline:

  • Moves root Terraform configuration to ./autogen
  • Creates submodule structure in ./modules and creates a public-cluster module

Copy link
Contributor

@aaron-lane aaron-lane left a comment

Choose a reason for hiding this comment

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

A few small changes requested but otherwise looks good.

Makefile Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
helpers/generate_modules/generate_modules.py Outdated Show resolved Hide resolved
helpers/generate_modules/generate_modules.py Outdated Show resolved Hide resolved
helpers/generate_modules/generate_modules.py Outdated Show resolved Hide resolved
helpers/generate_modules/generate_modules.py Outdated Show resolved Hide resolved
helpers/generate_modules/generate_modules.py Outdated Show resolved Hide resolved
test/helpers/generate_modules/test_generate_modules.py Outdated Show resolved Hide resolved
Copy link
Contributor

@morgante morgante left a comment

Choose a reason for hiding this comment

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

I think I might have been unclear on expectations here.

We should not be including complex Terraform-specific logic. Nor should we be causing a backwards-incompatible change to the root module.

Instead, this should be a pretty small and simple change:

  1. Copying all the root files into an autogen/ folder and making them into templates. We can use jinja2 or Go for templating (open to alternative suggestions).
  2. A very simple script which copies the files from the autogen folder into the specific modules. To start we will have just one: public. Keep the private in a follow-up PR.

To be clear, for this PR, we should avoid any symlinks.

autogen/main.tf Show resolved Hide resolved
main.tf Outdated Show resolved Hide resolved
outputs.tf Outdated Show resolved Hide resolved
test/helpers/generate_modules/test_generate_modules.py Outdated Show resolved Hide resolved
helpers/generate_modules/generate_modules.py Outdated Show resolved Hide resolved
@Jberlinsky
Copy link
Contributor Author

@morgante To be clear, the first iteration of this will be template generation from ./autogen into / (relative to the repository root), to avoid changing main.tf and triggering a state migration. Future PRs will generate additional module(s) into ./modules/. Is that your understanding?

@morgante
Copy link
Contributor

@Jberlinsky Yes, the addition of the private module can happen in a later PR.

Copy link
Contributor

@morgante morgante left a comment

Choose a reason for hiding this comment

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

We should also make sure the generated files include a header explaining they are autogenerated (and pointing to the autogen folder).

aaron-lane
aaron-lane previously approved these changes Jan 10, 2019
Copy link
Contributor

@morgante morgante left a comment

Choose a reason for hiding this comment

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

LGTM, merge once CI is green.

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

3 participants