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

Typed backend config #224

Merged
merged 10 commits into from
Jul 27, 2020
Merged

Conversation

jsteinich
Copy link
Collaborator

This adds the ability to specify Terraform Settings when creating a TerraformStack, including strongly typed backends.

I still need to add some test cases, but I believe it is otherwise complete. I think remote state belongs in a separate request.

@hashicorp-cla
Copy link

hashicorp-cla commented Jul 21, 2020

CLA assistant check
All committers have signed the CLA.

Copy link
Contributor

@skorfmann skorfmann left a comment

Choose a reason for hiding this comment

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

That's fantastic, thanks for your contribution.

Mid / Long term

I think the backends should follow the same concept as the TerraformProvider and would be ideally generated from a schema.

Since there are Go schemas for the backends (e.g. S3, I think that the backend classes should be generated from there somehow. This could be similar to what we do for provider Constructs and be a build step in cdktf.

The benefits I see for doing this:

  • No manual work to keep it aligned with Terraform
  • No need to test individual backends, as long as the code generator is tested properly
  • Inline docs would be generated as well
  • Long term maintenance effort significantly less compared to doing this manually

Perhaps the Terraform team could support this by exposing the Go schemas as JSON schema somehow? /cc @anubhavmishra

Short term

I think it'd be good to rework this PR to match the anticipated generated structre, which means:

  • Have a base TerraformBackend class
  • Treat backends as a Construct which get synthesised simlar to Resources, Data and Providers.
  • Drop support for generic TerraformSettings

This would mean, we'd be good to go with the manual implementation and could replace this later on by something which is generated from a JSON schema with little to none breaking changes

What are your thoughts @jsteinich / @anubhavmishra

examples/typescript-aws/main.ts Outdated Show resolved Hide resolved
packages/cdktf/lib/terraform-stack.ts Outdated Show resolved Hide resolved
packages/cdktf/lib/terraform-stack.ts Outdated Show resolved Hide resolved
@jsteinich
Copy link
Collaborator Author

I'm not a big fan of parsing the Go schemas directly, but if the Terraform team is willing to support getting them as JSON in some fashion, then that makes a lot of sense.

Making backends Constructs has some nice properties, but there is one point that I wonder about:
I like enforcing with the API that only 1 backend can be set. We could enforce in the synth or just let Terraform handle it though.

@skorfmann
Copy link
Contributor

I'm not a big fan of parsing the Go schemas directly, but if the Terraform team is willing to support getting them as JSON in some fashion, then that makes a lot of sense.

👍

Making backends Constructs has some nice properties, but there is one point that I wonder about:
I like enforcing with the API that only 1 backend can be set. We could enforce in the synth or just let Terraform handle it though.

Yes, this could be validated after synth. For now, I think it'd be sufficient to let Terraform do the validation.

@anubhavmishra
Copy link
Member

anubhavmishra commented Jul 21, 2020

Mid / Long term

I think the backends should follow the same concept as the TerraformProvider and would be ideally generated from a schema.

I think long term it makes sense to automatically generate the backends but for now, there is no way to do it.

Since there are Go schemas for the backends (e.g. S3, I think that the backend classes should be generated from there somehow. This could be similar to what we do for provider Constructs and be a build step in cdktf.

The benefits I see for doing this:

  • No manual work to keep it aligned with Terraform
  • No need to test individual backends, as long as the code generator is tested properly
  • Inline docs would be generated as well
  • Long term maintenance effort significantly less compared to doing this manually

Perhaps the Terraform team could support this by exposing the Go schemas as JSON schema somehow? /cc @anubhavmishra

We can have a conversation with the Terraform team to see if that is possible.

Short term

I think it'd be good to rework this PR to match the anticipated generated structre, which means:

  • Have a base TerraformBackend class
  • Treat backends as a Construct which get synthesised simlar to Resources, Data and Providers.
  • Drop support for generic TerraformSettings

This would mean, we'd be good to go with the manual implementation and could replace this later on by something which is generated from a JSON schema with little to none breaking changes

Backends don't tend to change as much as we should be good with the manual implementation for now.

@anubhavmishra
Copy link
Member

I edited the issue that was related to this PR. #33

@skorfmann
Copy link
Contributor

@jsteinich would you be up for refactoring this accordingly?

@jsteinich
Copy link
Collaborator Author

I should be able to get the changes done in the next couple of days.
I think remote state makes sense as a separate PR after this one.

@jsteinich jsteinich changed the title WIP: Terraform settings / typed backend config WIP: Typed backend config Jul 22, 2020
@skorfmann
Copy link
Contributor

I should be able to get the changes done in the next couple of days.
I think remote state makes sense as a separate PR after this one.

Awesome, thanks! Looking forward to it.

@jsteinich jsteinich changed the title WIP: Typed backend config Typed backend config Jul 24, 2020
@jsteinich
Copy link
Collaborator Author

I believe this is ready to go, but there are a few points that might require more discussion.

  1. I kept the code pretty minimal. I could see making some changes when/if switching to generated code, but I think having less code is better when manually maintained.
  2. I didn't transfer comments from the documentation. Seems like they would be useful longer term, but I'm not sure they add a ton right now.
  3. I think examples are going to need some more organization. I put the new examples in a folder structure that made some sense to me, but there are a few different ways this could be done. Can probably wait a bit on it though.

@jsteinich jsteinich mentioned this pull request Jul 24, 2020
@skorfmann
Copy link
Contributor

Cool, looks good at a first glance. Will look into it more thoroughly later today.

Copy link
Contributor

@skorfmann skorfmann left a comment

Choose a reason for hiding this comment

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

That looks very good, thank you! Two minor remarks as inline comments.

Other than that, could you adapt the docs about remote backends as well?

packages/cdktf/lib/backends/backend_cos.ts Outdated Show resolved Hide resolved
packages/cdktf/lib/terraform-backend.ts Show resolved Hide resolved
@skorfmann
Copy link
Contributor

skorfmann commented Jul 24, 2020 via email

Copy link
Contributor

@skorfmann skorfmann left a comment

Choose a reason for hiding this comment

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

Awesome, that looks great!

@github-actions
Copy link
Contributor

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've 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 Dec 15, 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