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

DXCDT-498: Add terraform generate command skeleton #792

Merged
merged 5 commits into from
Aug 8, 2023

Conversation

sergiught
Copy link
Contributor

@sergiught sergiught commented Aug 7, 2023

🔧 Changes

This PR adds the command skeleton for generating terraform config files for the Auth0 tenant.

📚 References

🔬 Testing

📝 Checklist

  • All new/changed/fixed functionality is covered by tests (or N/A)
  • I have added documentation for all new/changed functionality (or N/A)

@sergiught sergiught force-pushed the feature/DXCDT-498-terraform branch from e34ff05 to 9076052 Compare August 7, 2023 09:20
## Flags

```
-o, --output-dir string Output directory for the generated Terraform config files. If not provided, the files will be saved in the current working directory. (default "./")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I considered naming this just --dir, however the extra verbosity adds clarity in the full form for this flag and considering there's also the short form option of -o, I deemed unnecessary to have the flag shorter.


cmd := &cobra.Command{
Use: "generate",
Aliases: []string{"gen", "export"},
Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we are able to integrate the terraform-exec pkg as well, naming this generate makes more sense, otherwise keeping it as export would fit better. So for now I added both aliases. We can consider keeping just one of these once we explore how viable the terraform-exec pkg is for our use case of generating the config and state as well on behalf of the user.

Copy link
Contributor

@Widcket Widcket Aug 7, 2023

Choose a reason for hiding this comment

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

I'd suggest adding a comment so as to not forget to remove the lesser fitting alias in the future.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also keep in mind that once this goes out in a release, the aliases will be part of the public API, and removing one will be a breaking change. So you might want to consider not adding aliases until it's clear which one should be used.

Copy link
Contributor

Choose a reason for hiding this comment

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

A comment could also be used as a reminder for the above.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added a comment for now in 6cd31e8 (#792).

@sergiught sergiught marked this pull request as ready for review August 7, 2023 10:09
@sergiught sergiught requested a review from a team as a code owner August 7, 2023 10:09
internal/cli/terraform.go Outdated Show resolved Hide resolved
internal/cli/terraform.go Outdated Show resolved Hide resolved
required_providers {
auth0 = {
source = "auth0/auth0"
version = "1.0.0-beta.1"
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a way to set this value programmatically?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can't think of any. Any suggestions?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Once v1 GA goes live, we can consider omitting the version, so it always fetches the latest stable.

Copy link
Contributor

Choose a reason for hiding this comment

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

Once v1 GA goes live, we can consider omitting the version, so it always fetches the latest stable.

Once we eventually release v2 (e.g. in a couple of years), would this mean it will automatically fetch the new major (along with its breaking changes)?

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

All good things to consider, but at the moment we don't have any stable v1 so we can use "~> 1.0", and we need to specify the version explicitly for the beta so we can do some rapid testing as well. We'll adjust that value accordingly in the future.

@sergiught sergiught requested a review from Widcket August 7, 2023 10:38
@codecov-commenter
Copy link

codecov-commenter commented Aug 7, 2023

Codecov Report

Patch coverage: 82.43% and project coverage change: +0.06% 🎉

Comparison is base (35c1650) 72.04% compared to head (810de08) 72.11%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #792      +/-   ##
==========================================
+ Coverage   72.04%   72.11%   +0.06%     
==========================================
  Files          89       90       +1     
  Lines       11149    11223      +74     
==========================================
+ Hits         8032     8093      +61     
- Misses       2615     2627      +12     
- Partials      502      503       +1     
Files Changed Coverage Δ
internal/cli/terraform.go 82.19% <82.19%> (ø)
internal/cli/root.go 90.90% <100.00%> (+0.06%) ⬆️

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@sergiught sergiught requested a review from willvedd August 7, 2023 11:17
Copy link
Contributor

@willvedd willvedd left a comment

Choose a reason for hiding this comment

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

Mostly looking good but one question about an error check.

func terraformCmd(cli *cli) *cobra.Command {
cmd := &cobra.Command{
Use: "terraform",
Aliases: []string{"tf"},
Copy link
Contributor

Choose a reason for hiding this comment

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

I like the tf alias 👍

Long: "This command is designed to streamline the process of generating Terraform configuration files for " +
"your Auth0 resources, serving as a bridge between the two.\n\nIt automatically scans your Auth0 Tenant " +
"and compiles a set of Terraform configuration files based on the existing resources and configurations." +
"\n\nThe generated Terraform files are written in HashiCorp Configuration Language (HCL).",
Copy link
Contributor

Choose a reason for hiding this comment

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

Reminder for us to include instructions and/or link to a guide once one is available.

func generateTerraformConfigFiles(inputs *terraformInputs) error {
const readWritePermission = 0755
if err := os.MkdirAll(inputs.OutputDIR, readWritePermission); err != nil {
if !os.IsExist(err) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Does there need to be some nuance here? There's no certainty that an existing main.tf will be compatible with the command.

Some ideas on how to manage:

  • Require absence of main.tf
  • If main.tf exists, print warning explaining a potential incompatibility

Copy link
Contributor Author

@sergiught sergiught Aug 8, 2023

Choose a reason for hiding this comment

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

Good callout, however considering we will have the same issue with the import.tf file and the generated files, let's focus on this as a separate PR after those are added as well.

Currently, if the file exists it gets truncated and overwritten.

@sergiught sergiught merged commit 198c776 into main Aug 8, 2023
@sergiught sergiught deleted the feature/DXCDT-498-terraform branch August 8, 2023 11:44
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.

4 participants