Initialize tfexec.Terraform once per request + refactoring/cleanup #6630
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
kachawla
changed the title
kachawla
changed the title
kachawla
force-pushed
the
kachawla/terraform-improvements
branch
from
b523d6e to
cb91333
Compare
|
/ok-to-test |
Radius functional test overview
Click here to see the list of tools in the current test run
Test Status⌛ Building Radius and pushing container images for functional tests... |
Test Results2 975 tests - 1 2 963 ✔️ - 1 3m 4s ⏱️ +3s Results for commit f7666c2. ± Comparison against base commit c6a9049. This pull request removes 8 and adds 7 tests. Note that renamed tests count towards both.♻️ This comment has been updated with latest results. |
kachawla
force-pushed
the
kachawla/terraform-improvements
branch
from
cb91333 to
8db01f4
Compare
Signed-off-by: Karishma Chawla <[email protected]>
|
For the detailed report, please go to
|
Signed-off-by: Karishma Chawla <[email protected]>
kachawla
force-pushed
the
kachawla/terraform-improvements
branch
from
8db01f4 to
2dde25c
Compare
|
For the detailed report, please go to
|
|
/ok-to-test |
Radius functional test overview
Click here to see the list of tools in the current test run
Test Status⌛ Building Radius and pushing container images for functional tests... |
ytimocin
approved these changes
Nov 3, 2023
| // Create Working Directory | ||
| workingDir, err := createWorkingDir(ctx, options.RootDir) | ||
| // Create a new instance of tfexec.Terraform with current Terraform installation path | ||
| tf, err := NewTerraform(ctx, options.RootDir, execPath) |
There was a problem hiding this comment.
Could working in the Terraform installation path cause clashes with other files that will take place in the same directory?
Before we were creating a working directory (workingDir) in the root directory, now we will not create a working directory but will work in the execution path (execPath) -- the working directory and execution path will be the same. Would that cause any problems?
There was a problem hiding this comment.
We are still creating the working directory inside the root directory
radius/pkg/recipes/terraform/types.go
Line 67 in 2dde25c
| } | ||
|
|
||
| if err = tf.Get(ctx); err != nil { | ||
| func downloadModule(ctx context.Context, tf *tfexec.Terraform, templatePath string) error { |
There was a problem hiding this comment.
I think that, in the future, we could have a struct that has tfexec.Terraform as a property. That could have a constructor and some functions. One of the functions could be downloadModule, which could be something like this:
func (td *TerraformDriver) downloadModule(ctx context.Context, templatePath string) error {
td.Terraform.Get(ctx)...
...
}
Note: The struct name will probably something other than TerraformDriver :)
There was a problem hiding this comment.
This way we could also mock some of the functions of tfexec.Terraform.
There was a problem hiding this comment.
That makes sense to me. Some of the functions called from execute.go can be cleaned up as well downloadModule, inspectModule etc, but it's best to do that in smaller PRs so didn't include that here.
|
For the detailed report, please go to
|
sk593
approved these changes
Nov 3, 2023
willdavsmith
pushed a commit
to willdavsmith/radius
that referenced
this pull request
Nov 3, 2023
…adius-project#6630) # Description * Terraform instance along with logging configuration is being called multiple times for same request - consolidating it to be initialized only once. * Move working directory creation to tfexec.Terraform initialization to remove duplicated code. ## Type of change <!-- Please select **one** of the following options that describes your change and delete the others. Clearly identifying the type of change you are making will help us review your PR faster, and is used in authoring release notes. If you are making a bug fix or functionality change to Radius and do not have an associated issue link please create one now. --> - This pull request fixes a bug in Radius and has an approved issue (issue link required). <!-- Please update the following to link the associated issue. This is required for some kinds of changes (see above). --> Fixes: radius-project#6456 ## Auto-generated summary <!-- GitHub Copilot for docs will auto-generate a summary of the PR --> <!-- copilot:all --> ### <samp>🤖 Generated by Copilot at b523d6e</samp> ### Summary 📝🛠️🚀 <!-- 1. 📝 - This emoji represents the addition of the log.go file and the types_test.go file, which implement and test new features for the terraform package. The emoji also represents the refactoring of the test cases and the error messages in the package, which improve the documentation and clarity of the code. 2. 🛠️ - This emoji represents the refactoring of the terraform package in execute.go and module_test.go, which use the tfexec package and the new functions in types.go to simplify the function signatures and reduce the duplication of parameters. The emoji also represents the fixing of the import alias and the resource types in the show command test cases, which improve the consistency and readability of the code. 3. 🚀 - This emoji represents the enhancement of the terraform package in types.go, which adds functions and constants to create and configure a working directory for Terraform execution and logging. The emoji also represents the use of the tfLogWrapper struct in log.go, which streams the Terraform logs to the Radius logger. These features improve the performance and user experience of the terraform package. --> Refactored the terraform package to use the `tfexec.Terraform` type and a custom logger. Improved the consistency and readability of the tests and the code. Added new files `log.go` and `types_test.go` to implement and test the new features. > _Oh we're the `terraform` crew and we've got work to do_ > _We'll refactor and improve our code with `tfexec` too_ > _We'll stream our logs and test our types and make our imports clear_ > _So heave away, me hearties, heave away with cheer_ ### Walkthrough * Fix import alias for datastoresrp package in `show_test.go` ([link](https://github.com/radius-project/radius/pull/6630/files?diff=unified&w=0#diff-13fc0c59e89a1fa92926d4fdd0ef9a6ef372c9816c6c43ec6b003427b63e0852L33-R33), [link](https://github.com/radius-project/radius/pull/6630/files?diff=unified&w=0#diff-13fc0c59e89a1fa92926d4fdd0ef9a6ef372c9816c6c43ec6b003427b63e0852L49-R49), [link](https://github.com/radius-project/radius/pull/6630/files?diff=unified&w=0#diff-13fc0c59e89a1fa92926d4fdd0ef9a6ef372c9816c6c43ec6b003427b63e0852L58-R58), [link](https://github.com/radius-project/radius/pull/6630/files?diff=unified&w=0#diff-13fc0c59e89a1fa92926d4fdd0ef9a6ef372c9816c6c43ec6b003427b63e0852L67-R67), [link](https://github.com/radius-project/radius/pull/6630/files?diff=unified&w=0#diff-13fc0c59e89a1fa92926d4fdd0ef9a6ef372c9816c6c43ec6b003427b63e0852L76-R76), [link](https://github.com/radius-project/radius/pull/6630/files?diff=unified&w=0#diff-13fc0c59e89a1fa92926d4fdd0ef9a6ef372c9816c6c43ec6b003427b63e0852L114-R114), [link](https://github.com/radius-project/radius/pull/6630/files?diff=unified&w=0#diff-13fc0c59e89a1fa92926d4fdd0ef9a6ef372c9816c6c43ec6b003427b63e0852L148-R148), [link](https://github.com/radius-project/radius/pull/6630/files?diff=unified&w=0#diff-13fc0c59e89a1fa92926d4fdd0ef9a6ef372c9816c6c43ec6b003427b63e0852L190-R190), [link](https://github.com/radius-project/radius/pull/6630/files?diff=unified&w=0#diff-13fc0c59e89a1fa92926d4fdd0ef9a6ef372c9816c6c43ec6b003427b63e0852L225-R225)) * Refactor Terraform executor creation and configuration in `types.go` and `log.go` ([link](https://github.com/radius-project/radius/pull/6630/files?diff=unified&w=0#diff-0ad85ed086350c77c3e366408120b4c3020d1cec542671ab3586fd0c34dafe6fL22-R25), [link](https://github.com/radius-project/radius/pull/6630/files?diff=unified&w=0#diff-0ad85ed086350c77c3e366408120b4c3020d1cec542671ab3586fd0c34dafe6fL30-R37), [link](https://github.com/radius-project/radius/pull/6630/files?diff=unified&w=0#diff-0ad85ed086350c77c3e366408120b4c3020d1cec542671ab3586fd0c34dafe6fL59-R71), [link](https://github.com/radius-project/radius/pull/6630/files?diff=unified&w=0#diff-0ad85ed086350c77c3e366408120b4c3020d1cec542671ab3586fd0c34dafe6fL71-R92), [link](https://github.com/radius-project/radius/pull/6630/files?diff=unified&w=0#diff-cd97896943008b717cf4f1a771f6dd35263008aebb0ebccb272755793d10a1acR1-R87), [link](https://github.com/radius-project/radius/pull/6630/files?diff=unified&w=0#diff-8242ca1f206aeefb27fdc2d73686357bf7cb70572c1003c0586e2ba189a6e7f6R1-R56)) * Simplify generateConfig, initAndApply, initAndDestroy, downloadAndInspect, and downloadModule functions to accept a Terraform executor instead of a working directory and an exec path in `execute.go` and `module.go` ([link](https://github.com/radius-project/radius/pull/6630/files?diff=unified&w=0#diff-41ec31c0ceb8bd50c0329b55a8d27aeba69648059faef4e785302155f4e97482L23-R26), [link](https://github.com/radius-project/radius/pull/6630/files?diff=unified&w=0#diff-41ec31c0ceb8bd50c0329b55a8d27aeba69648059faef4e785302155f4e97482L45-L49), [link](https://github.com/radius-project/radius/pull/6630/files?diff=unified&w=0#diff-41ec31c0ceb8bd50c0329b55a8d27aeba69648059faef4e785302155f4e97482L92-R86), [link](https://github.com/radius-project/radius/pull/6630/files?diff=unified&w=0#diff-41ec31c0ceb8bd50c0329b55a8d27aeba69648059faef4e785302155f4e97482L99-R92), [link](https://github.com/radius-project/radius/pull/6630/files?diff=unified&w=0#diff-41ec31c0ceb8bd50c0329b55a8d27aeba69648059faef4e785302155f4e97482L105-R98), [link](https://github.com/radius-project/radius/pull/6630/files?diff=unified&w=0#diff-41ec31c0ceb8bd50c0329b55a8d27aeba69648059faef4e785302155f4e97482L141-R135), [link](https://github.com/radius-project/radius/pull/6630/files?diff=unified&w=0#diff-41ec31c0ceb8bd50c0329b55a8d27aeba69648059faef4e785302155f4e97482L148-R141), [link](https://github.com/radius-project/radius/pull/6630/files?diff=unified&w=0#diff-41ec31c0ceb8bd50c0329b55a8d27aeba69648059faef4e785302155f4e97482L168-R161), [link](https://github.com/radius-project/radius/pull/6630/files?diff=unified&w=0#diff-41ec31c0ceb8bd50c0329b55a8d27aeba69648059faef4e785302155f4e97482L201-R205), [link](https://github.com/radius-project/radius/pull/6630/files?diff=unified&w=0#diff-41ec31c0ceb8bd50c0329b55a8d27aeba69648059faef4e785302155f4e97482L222-R218), [link](https://github.com/radius-project/radius/pull/6630/files?diff=unified&w=0#diff-41ec31c0ceb8bd50c0329b55a8d27aeba69648059faef4e785302155f4e97482L243-R225), [link](https://github.com/radius-project/radius/pull/6630/files?diff=unified&w=0#diff-41ec31c0ceb8bd50c0329b55a8d27aeba69648059faef4e785302155f4e97482L301-R283), [link](https://github.com/radius-project/radius/pull/6630/files?diff=unified&w=0#diff-41ec31c0ceb8bd50c0329b55a8d27aeba69648059faef4e785302155f4e97482L307-R289), [link](https://github.com/radius-project/radius/pull/6630/files?diff=unified&w=0#diff-41ec31c0ceb8bd50c0329b55a8d27aeba69648059faef4e785302155f4e97482L320-R302), [link](https://github.com/radius-project/radius/pull/6630/files?diff=unified&w=0#diff-41ec31c0ceb8bd50c0329b55a8d27aeba69648059faef4e785302155f4e97482L353-R339), [link](https://github.com/radius-project/radius/pull/6630/files?diff=unified&w=0#diff-41ec31c0ceb8bd50c0329b55a8d27aeba69648059faef4e785302155f4e97482L385-R371), [link](https://github.com/radius-project/radius/pull/6630/files?diff=unified&w=0#diff-4f4cb2009f77e8f5e252eee95f3ed880148fc285798ef4b7bbeb970525ad7b5aR25), [link](https://github.com/radius-project/radius/pull/6630/files?diff=unified&w=0#diff-4f4cb2009f77e8f5e252eee95f3ed880148fc285798ef4b7bbeb970525ad7b5aL101-R103)) * Update TestGeneratedConfig, Test_GetTerraformConfig, Test_GetTerraformConfig_EmptyRecipeName, and Test_GetTerraformConfig_InvalidDirectory functions to use the NewTerraform function and pass the test directory instead of the working directory in `execute_test.go` ([link](https://github.com/radius-project/radius/pull/6630/files?diff=unified&w=0#diff-3b5890b2cb244038dacda842a1e285235330c421ad27e8bc02d6f979ee8869caL20-R23), [link](https://github.com/radius-project/radius/pull/6630/files?diff=unified&w=0#diff-3b5890b2cb244038dacda842a1e285235330c421ad27e8bc02d6f979ee8869caL30-R30), [link](https://github.com/radius-project/radius/pull/6630/files?diff=unified&w=0#diff-3b5890b2cb244038dacda842a1e285235330c421ad27e8bc02d6f979ee8869caL78-R38), [link](https://github.com/radius-project/radius/pull/6630/files?diff=unified&w=0#diff-3b5890b2cb244038dacda842a1e285235330c421ad27e8bc02d6f979ee8869caL85-L105), [link](https://github.com/radius-project/radius/pull/6630/files?diff=unified&w=0#diff-3b5890b2cb244038dacda842a1e285235330c421ad27e8bc02d6f979ee8869caL115-R58), [link](https://github.com/radius-project/radius/pull/6630/files?diff=unified&w=0#diff-3b5890b2cb244038dacda842a1e285235330c421ad27e8bc02d6f979ee8869caL128-L129), [link](https://github.com/radius-project/radius/pull/6630/files?diff=unified&w=0#diff-3b5890b2cb244038dacda842a1e285235330c421ad27e8bc02d6f979ee8869caL142-R81), [link](https://github.com/radius-project/radius/pull/6630/files?diff=unified&w=0#diff-3b5890b2cb244038dacda842a1e285235330c421ad27e8bc02d6f979ee8869caL151-L152), [link](https://github.com/radius-project/radius/pull/6630/files?diff=unified&w=0#diff-3b5890b2cb244038dacda842a1e285235330c421ad27e8bc02d6f979ee8869caL161-R98), [link](https://github.com/radius-project/radius/pull/6630/files?diff=unified&w=0#diff-3b5890b2cb244038dacda842a1e285235330c421ad27e8bc02d6f979ee8869caL167-R104), [link](https://github.com/radius-project/radius/pull/6630/files?diff=unified&w=0#diff-3b5890b2cb244038dacda842a1e285235330c421ad27e8bc02d6f979ee8869caL179-R115)) * Remove Test_DownloadModule_EmptyWorkingDirPath_Error test case as it is no longer relevant in `module_test.go` ([link](https://github.com/radius-project/radius/pull/6630/files?diff=unified&w=0#diff-35f143c273df8b35733ba5b5e44bc5bb5e4dbe8467aeabc3e861b5e19354791bL20-R22), [link](https://github.com/radius-project/radius/pull/6630/files?diff=unified&w=0#diff-35f143c273df8b35733ba5b5e44bc5bb5e4dbe8467aeabc3e861b5e19354791bL92-L101)) * Rename test case for empty recipe name error in `execute_test.go` ([link](https://github.com/radius-project/radius/pull/6630/files?diff=unified&w=0#diff-3b5890b2cb244038dacda842a1e285235330c421ad27e8bc02d6f979ee8869caL78-R38)) * Change test directory name and error message in Test_GetTerraformConfig_InvalidDirectory function in `execute_test.go` ([link](https://github.com/radius-project/radius/pull/6630/files?diff=unified&w=0#diff-3b5890b2cb244038dacda842a1e285235330c421ad27e8bc02d6f979ee8869caL167-R104), [link](https://github.com/radius-project/radius/pull/6630/files?diff=unified&w=0#diff-3b5890b2cb244038dacda842a1e285235330c421ad27e8bc02d6f979ee8869caL179-R115)) --------- Signed-off-by: Karishma Chawla <[email protected]> Co-authored-by: karishma-chawla <[email protected]> Signed-off-by: willdavsmith <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Terraform instance along with logging configuration is being called multiple times for same request - consolidating it to be initialized only once.
Move working directory creation to tfexec.Terraform initialization to remove duplicated code.
Type of change
Fixes: #6456
Auto-generated summary
🤖 Generated by Copilot at b523d6e
Summary
📝🛠️🚀
Refactored the terraform package to use the
tfexec.Terraformtype and a custom logger. Improved the consistency and readability of the tests and the code. Added new fileslog.goandtypes_test.goto implement and test the new features.Walkthrough
show_test.go(link, link, link, link, link, link, link, link, link)types.goandlog.go(link, link, link, link, link, link)execute.goandmodule.go(link, link, link, link, link, link, link, link, link, link, link, link, link, link, link, link, link, link)execute_test.go(link, link, link, link, link, link, link, link, link, link, link)module_test.go(link, link)execute_test.go(link)execute_test.go(link, link)