Add check for Terraform submodules #7013
Conversation
pkg/recipes/terraform/module.go
Outdated
| result := &moduleInspectResult{ContextVarExists: false, RequiredProviders: []string{}, ResultOutputExists: false, Parameters: map[string]any{}} | ||
|
|
||
| // Modules are downloaded in a subdirectory in the working directory. | ||
| // Name of the module specified in the configuration is used as subdirectory name. | ||
| // https://developer.hashicorp.com/terraform/tutorials/modules/module-use#understand-how-modules-work | ||
| // | ||
| // If the template path is for a submodule, we'll add the submodule path to the module root directory. | ||
| if strings.Contains(localModuleName, "//") { |
There was a problem hiding this comment.
Terraform has a hard convention for the double slash: https://developer.hashicorp.com/terraform/language/modules/sources#modules-in-package-sub-directories
We can use this special character to check if the module source is a submodule
There was a problem hiding this comment.
Updated this to use an existing package implementation: https://github.com/hashicorp/go-getter/blob/main/source.go#L17
sk593
temporarily deployed
to
functional-tests
GitHub Actions
Inactive
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... |
sk593
force-pushed
the
tf-submodules
branch
2 times, most recently
from
9abc501 to
0b8a53e
Compare
sk593
changed the title
|
can we add a functional test for this scenario? |
pkg/recipes/terraform/execute.go
Outdated
| @@ -281,7 +281,7 @@ func downloadAndInspect(ctx context.Context, tf *tfexec.Terraform, options Optio | |||
| // Load the downloaded module to retrieve providers and variables required by the module. | |||
| // This is needed to add the appropriate providers config and populate the value of recipe context variable. | |||
| logger.Info(fmt.Sprintf("Inspecting the downloaded Terraform module: %s", options.EnvRecipe.TemplatePath)) | |||
| loadedModule, err := inspectModule(tf.WorkingDir(), options.EnvRecipe.Name) | |||
| loadedModule, err := inspectModule(tf.WorkingDir(), options.EnvRecipe.Name, options.EnvRecipe.TemplatePath) | |||
There was a problem hiding this comment.
Just thinking out loud; would it make more sense to have the whole Recipe object as the input to the inspectModule function? For future proofing the function... Instead of increasing the number of parameters, we can have a single parameter that would contain all the necessary.
There was a problem hiding this comment.
Updated to use the recipe
sk593
temporarily deployed
to
functional-tests
GitHub Actions
Inactive
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... |
sk593
temporarily deployed
to
functional-tests
GitHub Actions
Inactive
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... |
| @@ -45,6 +45,7 @@ require ( | |||
| github.com/google/go-cmp v0.5.9 | |||
| github.com/google/uuid v1.3.0 | |||
| github.com/gosuri/uilive v0.0.4 | |||
| github.com/hashicorp/go-retryablehttp v0.7.5 | |||
There was a problem hiding this comment.
We can run go mod tidy just in case.
There was a problem hiding this comment.
I'll rerun again before merging to make sure this is up to date
I'm planning to address this in this PR, but I'm still working through the best way to upload the submodule folder. Since we compress our recipes and call them over HTTPS, I don't think it will work exactly the same as it does now |
sk593
dismissed stale reviews from lakshmimsft, kachawla, and ytimocin
via
3e307a9
sk593
temporarily deployed
to
functional-tests
GitHub Actions
Inactive
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... |
| @@ -21,7 +21,7 @@ resource env 'Applications.Core/environments@2023-10-01-preview' = { | |||
| 'Applications.Core/extenders': { | |||
| default: { | |||
| templateKind: 'terraform' | |||
| templatePath: '${moduleServer}/kubernetes-redis.zip' | |||
| templatePath: '${moduleServer}/kubernetes-redis.zip//kubernetes-redis/modules' | |||
There was a problem hiding this comment.
Looking at the docs https://docs.python.org/3/library/shutil.html#shutil-archiving-example-with-basedir, I think unpack returns directories inside root directory. So we either change the path here to //modules or specify base directory while creating the zip here: https://github.com/radius-project/radius/blob/main/.github/scripts/publish-test-terraform-recipes.py#L63 - root dir would change to tmp_dir and base directory would be recipe_dir
There was a problem hiding this comment.
Updated to //modules and that seems to have fixed the path issue. Thanks for finding this!
sk593
temporarily deployed
to
functional-tests
GitHub Actions
Inactive
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... |
Signed-off-by: sk593 <[email protected]>
Signed-off-by: sk593 <[email protected]>
Signed-off-by: sk593 <[email protected]>
Signed-off-by: sk593 <[email protected]>
…adius-project#7067) # Description `Test_Run_Logger` uses `debian` image for testing. This can encounter the rate limit problem during long-running test. This change uses magpie image to prevent this failure. ## Type of change - This pull request is a minor refactor, code cleanup, test improvement, or other maintenance task and doesn't change the functionality of Radius (issue link optional). Signed-off-by: Young Bu Park <[email protected]> Signed-off-by: sk593 <[email protected]>
…t#6889) # Description Return an error if application properties or properties.environment have not been specified ## 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#6882 ## Auto-generated summary <!-- GitHub Copilot for docs will auto-generate a summary of the PR --> <!-- copilot:all --> ### <samp>🤖[[deprecated]](https://githubnext.com/copilot-for-prs-sunset) Generated by Copilot at e982e0a</samp> ### Summary :sparkles::bug::wastebasket: <!-- 1. :sparkles: - This emoji represents the addition of new features or enhancements, such as the new custom actions, the new approval workflow, the new API version, and the new test cases. 2. :bug: - This emoji represents the fixing of bugs or errors, such as the error handling for the application conversion, and the typo in the bug report body. 3. :wastebasket: - This emoji represents the removal of unused or deprecated code, such as the `/ok-to-test` command and its related functions. --> This pull request introduces a new approval mechanism for the functional tests workflow, a new API version for the application resource with new validations, and some error handling and testing improvements. The pull request modifies the `radius-bot.js` script, the `.github` folder, the `docs` folder, and the `pkg/corerp` folder. The pull request aims to enhance the security, quality, and usability of the radius project. > _To test PRs with more care_ > _We added approval and error_ > _We saved PR number_ > _With a custom actioner_ > _And fixed a typo in bug reporter_ ### Walkthrough * Add a new API version for the application resource with new features and validations ([link](https://github.com/radius-project/radius/pull/6889/files?diff=unified&w=0#diff-d2cea876bc0e0849a386e79c8cb75b19c1a69022482c4c09b683ed4651fd673fR29-R37), [link](https://github.com/radius-project/radius/pull/6889/files?diff=unified&w=0#diff-dc0244afef9b2619d0a7837f85235bf32273a15673eb4ba1271b0ebb91f21728R51-R60), [link](https://github.com/radius-project/radius/pull/6889/files?diff=unified&w=0#diff-219a76ac16a85c0d41806b70a3554a25882b7ba8a132da7bfe169d2330e7d51cL1-R23), [link](https://github.com/radius-project/radius/pull/6889/files?diff=unified&w=0#diff-87c7befd7dc89411996a7e8259a3b11e586c14d1c80a823c863f0d069f215c17L1-R2), [link](https://github.com/radius-project/radius/pull/6889/files?diff=unified&w=0#diff-c69758755a898a2bf2a9e897278d1fbf63138f41cba6c166c11fd775aab89c53R49-R51)) - Validate that the properties and environment fields are not nil in the versioned model `pkg/corerp/api/v20231001preview/application_conversion.go` ([link](https://github.com/radius-project/radius/pull/6889/files?diff=unified&w=0#diff-d2cea876bc0e0849a386e79c8cb75b19c1a69022482c4c09b683ed4651fd673fR29-R37)) - Return an error if the conversion from the versioned to the data model fails in `pkg/corerp/datamodel/converter/application_converter.go` ([link](https://github.com/radius-project/radius/pull/6889/files?diff=unified&w=0#diff-c69758755a898a2bf2a9e897278d1fbf63138f41cba6c166c11fd775aab89c53R49-R51)) - Add test cases and test data files for the application conversion logic in `pkg/corerp/api/v20231001preview/application_conversion_test.go` and `pkg/corerp/api/v20231001preview/testdata` ([link](https://github.com/radius-project/radius/pull/6889/files?diff=unified&w=0#diff-dc0244afef9b2619d0a7837f85235bf32273a15673eb4ba1271b0ebb91f21728R51-R60), [link](https://github.com/radius-project/radius/pull/6889/files?diff=unified&w=0#diff-219a76ac16a85c0d41806b70a3554a25882b7ba8a132da7bfe169d2330e7d51cL1-R23), [link](https://github.com/radius-project/radius/pull/6889/files?diff=unified&w=0#diff-87c7befd7dc89411996a7e8259a3b11e586c14d1c80a823c863f0d069f215c17L1-R2)) * Change the trigger and approval process for the functional tests workflow ([link](https://github.com/radius-project/radius/pull/6889/files?diff=unified&w=0#diff-b94c0e124b5b65fb6fa8785928c5331547c124a96ecc225f93df9445a29a5109L47-L49), [link](https://github.com/radius-project/radius/pull/6889/files?diff=unified&w=0#diff-b94c0e124b5b65fb6fa8785928c5331547c124a96ecc225f93df9445a29a5109L79-L144), [link](https://github.com/radius-project/radius/pull/6889/files?diff=unified&w=0#diff-c79f364a9293abaaa8595776b74674e24bec6287834e63ab8aa7aec6a42f0dbcL26-R30), [link](https://github.com/radius-project/radius/pull/6889/files?diff=unified&w=0#diff-c79f364a9293abaaa8595776b74674e24bec6287834e63ab8aa7aec6a42f0dbcL98-R118), [link](https://github.com/radius-project/radius/pull/6889/files?diff=unified&w=0#diff-336842f9ad3be6e33759449fb701d721641268b444401b71c63a4e371ae91598R1-R17), [link](https://github.com/radius-project/radius/pull/6889/files?diff=unified&w=0#diff-fb0f7798935160052ab4612b954c50fb8f31d3825cce346f5feb693c418f2abaR83-R84)) - Remove the /ok-to-test command and its logic from the issue comment create event handler and the functions in `pkg/corerp/scripts/radius-bot.js` ([link](https://github.com/radius-project/radius/pull/6889/files?diff=unified&w=0#diff-b94c0e124b5b65fb6fa8785928c5331547c124a96ecc225f93df9445a29a5109L47-L49), [link](https://github.com/radius-project/radius/pull/6889/files?diff=unified&w=0#diff-b94c0e124b5b65fb6fa8785928c5331547c124a96ecc225f93df9445a29a5109L79-L144)) - Add a new workflow for approving the functional tests run for a pull request in `.github/workflows/functional-tests-approval.yaml` ([link](https://github.com/radius-project/radius/pull/6889/files?diff=unified&w=0#diff-336842f9ad3be6e33759449fb701d721641268b444401b71c63a4e371ae91598R1-R17)) - Modify the trigger conditions and the steps for setting up the checkout target for the functional tests workflow in `.github/workflows/functional-test.yaml` ([link](https://github.com/radius-project/radius/pull/6889/files?diff=unified&w=0#diff-c79f364a9293abaaa8595776b74674e24bec6287834e63ab8aa7aec6a42f0dbcL26-R30), [link](https://github.com/radius-project/radius/pull/6889/files?diff=unified&w=0#diff-c79f364a9293abaaa8595776b74674e24bec6287834e63ab8aa7aec6a42f0dbcL98-R118)) - Add a documentation section to explain the new approval process for the functional tests workflow in `docs/contributing/contributing-pull-requests/README.md` ([link](https://github.com/radius-project/radius/pull/6889/files?diff=unified&w=0#diff-fb0f7798935160052ab4612b954c50fb8f31d3825cce346f5feb693c418f2abaR83-R84)) * Add custom actions to save and download the PR number as an artifact ([link](https://github.com/radius-project/radius/pull/6889/files?diff=unified&w=0#diff-47021e538b3451866b8d8aa608664649ca716a99718bbaa9472a3af67dcd0610R1-R43), [link](https://github.com/radius-project/radius/pull/6889/files?diff=unified&w=0#diff-a06147d654903226b1bb5a4635b89fa5f7e0bd2168a3590f05bdbe65c2c0e2d4R1-R17)) - Add an action to save the PR number as an artifact in `.github/actions/save-pr-as-artifact/action.yaml` ([link](https://github.com/radius-project/radius/pull/6889/files?diff=unified&w=0#diff-a06147d654903226b1bb5a4635b89fa5f7e0bd2168a3590f05bdbe65c2c0e2d4R1-R17)) - Add an action to download the PR number from the artifact in `.github/actions/download-pr-data-artifact/action.yaml` ([link](https://github.com/radius-project/radius/pull/6889/files?diff=unified&w=0#diff-47021e538b3451866b8d8aa608664649ca716a99718bbaa9472a3af67dcd0610R1-R43)) * Fix a typo in the bug report body generated by the functional tests workflow ([link](https://github.com/radius-project/radius/pull/6889/files?diff=unified&w=0#diff-c79f364a9293abaaa8595776b74674e24bec6287834e63ab8aa7aec6a42f0dbcL579-L578)) --------- Signed-off-by: vinayada1 <[email protected]> Signed-off-by: sk593 <[email protected]>
Signed-off-by: sk593 <[email protected]>
sk593
temporarily deployed
to
functional-tests
GitHub Actions
Inactive
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... |
# Description This checks for submodules when deploying TF recipes. If the source is a submodule, we'll use that path when loading the module after the download ## 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 is a minor refactor, code cleanup, test improvement, or other maintenance task and doesn't change the functionality of Radius radius-project#7005 <!-- Please update the following to link the associated issue. This is required for some kinds of changes (see above). --> Fixes: radius-project#7005 --------- Signed-off-by: sk593 <[email protected]> Signed-off-by: Young Bu Park <[email protected]> Signed-off-by: vinayada1 <[email protected]> Co-authored-by: Young Bu Park <[email protected]> Co-authored-by: vinayada1 <[email protected]>
# Description This checks for submodules when deploying TF recipes. If the source is a submodule, we'll use that path when loading the module after the download ## 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 is a minor refactor, code cleanup, test improvement, or other maintenance task and doesn't change the functionality of Radius radius-project#7005 <!-- Please update the following to link the associated issue. This is required for some kinds of changes (see above). --> Fixes: radius-project#7005 --------- Signed-off-by: sk593 <[email protected]> Signed-off-by: Young Bu Park <[email protected]> Signed-off-by: vinayada1 <[email protected]> Co-authored-by: Young Bu Park <[email protected]> Co-authored-by: vinayada1 <[email protected]>
Description
This checks for submodules when deploying TF recipes. If the source is a submodule, we'll use that path when loading the module after the download
Type of change
Fixes: #7005