-
Notifications
You must be signed in to change notification settings - Fork 96
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
[Draft] Implementing Terraform driver #5533
Conversation
You are editing files which require a docs update. Please ensure you've made the appropriate changes to the docs and submitted a PR.
For more information on contributing to docs please visit https://docs.radapp.dev/contributing/docs/. |
Test Results2 541 tests +3 2 534 ✔️ +3 1m 55s ⏱️ +2s Results for commit ecec4a9. ± Comparison against base commit 473dbdf. This pull request removes 2 and adds 5 tests. Note that renamed tests count towards both.
♻️ This comment has been updated with latest results. |
065f952
to
432d6bf
Compare
You are editing files which require a docs update. Please ensure you've made the appropriate changes to the docs and submitted a PR.
For more information on contributing to docs please visit https://docs.radapp.dev/contributing/docs/. |
432d6bf
to
b052369
Compare
You are editing files which require a docs update. Please ensure you've made the appropriate changes to the docs and submitted a PR.
For more information on contributing to docs please visit https://docs.radapp.dev/contributing/docs/. |
b052369
to
abdfb9e
Compare
You are editing files which require a docs update. Please ensure you've made the appropriate changes to the docs and submitted a PR.
For more information on contributing to docs please visit https://docs.radapp.dev/contributing/docs/. |
abdfb9e
to
161b71e
Compare
You are editing files which require a docs update. Please ensure you've made the appropriate changes to the docs and submitted a PR.
For more information on contributing to docs please visit https://docs.radapp.dev/contributing/docs/. |
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 test... |
@@ -363,7 +363,8 @@ | |||
## EnvironmentRecipeProperties | |||
### Properties | |||
* **parameters**: any: Any object | |||
* **templatePath**: string (Required): Path to the template provided by the recipe. Currently only link to Azure Container Registry is supported. | |||
* **templateKind**: string: Format of the template provided by the recipe. Currenlty only Bicep and Terraform templates are supported. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be possible to separate the app model change from the other changes so we can get it in sooner?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was a quick solution for me to be able to integrate end to end. I expect it to be added separately as a required field (which will result into updating all bicep templates as well) - have a separate task tracking that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Went ahead and separated it out here #5575, since it will be good to get users onboarded to this change sooner than later as you mentioned as well, so targeting to include it in the release next week.
...tions/resource-manager/Applications.Core/preview/2022-03-15-privatepreview/environments.json
Outdated
Show resolved
Hide resolved
@@ -63,12 +63,16 @@ spec: | |||
volumeMounts: | |||
- name: config-volume | |||
mountPath: /etc/config | |||
- name: terraform | |||
mountPath: /terraform |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Something to consider here - how does the RP know to use this directory? Is this configurable?
|
||
const ( | ||
installDirRoot = "/terraform/install" | ||
workingDirRoot = "/terraform/exec" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we find a way to make this configurable? This will cause problems if these paths are used in dev scenarios.
// NewTerraformDriver creates a new instance of driver to execute a Terraform recipe. | ||
func NewTerraformDriver() Driver { | ||
return &terraformDriver{} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: There's no need to define a "constructor" unless there's some initialization to do.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not yet, but we will likely initialize things needed to setup creds.
} | ||
|
||
type TerraformDefinition struct { | ||
RequiredProviders map[string]interface{} `json:"required_providers"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use camelCase for json property names
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is driven by Terraform's syntax so we don't really have much of a control in what it should look like. I'll test if Terraform accepts camel casing, haven't seen that so far.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Checked if Terraform accepts camelCase, it does not. Let me know if I misunderstood your comment.
The Terraform configuration must be valid before initialization so that
Terraform can determine which modules and providers need to be installed.
╷
│ Error: Extraneous JSON object property
│
│ on main.tf.json line 3, in terraform:
│ 3: "requiredProviders": {
│
│ No argument or block type is named "requiredProviders". Did you mean "required_providers"?
╵
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AH I get it. If this is an existing format we should obviously follow that 😆
logger.Info(fmt.Sprintf("Deploying recipe: %q, template: %q", recipe.Name, definition.TemplatePath)) | ||
|
||
// Create Terraform installation directory | ||
installDir := filepath.Join(installDirRoot, util.NormalizeStringToLower(recipe.ResourceID), uuid.NewString()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this to create a unique install directory for each execution of the driver? Can you leave a comment explaining why instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this is emphemeral and based on a UUID - why do we need to use the resource ID at all? Is a UUID not sufficiently unique?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had added a comment regarding this below before Install
is called - I don't know yet if we need unique installation per request or we should reuse the installation. We can optimize this as we learn more.
// We should look into checking if an exsiting installation of Terraform is available.
// For initial iteration we will always install Terraform. Optimizations can be made later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this is emphemeral and based on a UUID - why do we need to use the resource ID at all? Is a UUID not sufficiently unique?
Not for uniqueness, but for tracking and debugging if we ever need to figure out which resource the directory was created for.
pkg/recipes/driver/terraform.go
Outdated
Version string `json:"version"` | ||
} | ||
|
||
func (d *terraformDriver) Execute(ctx context.Context, configuration recipes.Configuration, recipe recipes.Metadata, definition recipes.Definition) (recipeOutput *recipes.RecipeOutput, err error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Its not too early to start thinking about testability. There's a lot of code in this function that does IO and can't easily be compartmentalized for unit testing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For example you could pull the installer into an interface or separate function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is first pass at implementation to get us to a state where we can quickly figure out if there are any unknown unknowns we haven't come across yet. So I haven't really spent time on optimizing for testability, polishing up the code, and making values configurable (that you mentioned in other comments).
You are editing files which require a docs update. Please ensure you've made the appropriate changes to the docs and submitted a PR.
For more information on contributing to docs please visit https://docs.radapp.dev/contributing/docs/. |
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 test... |
1abf860
to
0aec081
Compare
You are editing files which require a docs update. Please ensure you've made the appropriate changes to the docs and submitted a PR.
For more information on contributing to docs please visit https://docs.radapp.dev/contributing/docs/. |
This comment has been minimized.
This comment has been minimized.
⌛ Building Radius and pushing container images for functional tests... ✅ Container images build succeeded |
0aec081
to
fbab6b2
Compare
You are editing files which require a docs update. Please ensure you've made the appropriate changes to the docs and submitted a PR.
For more information on contributing to docs please visit https://docs.radapp.dev/contributing/docs/. |
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... |
fbab6b2
to
7e1042e
Compare
You are editing files which require a docs update. Please ensure you've made the appropriate changes to the docs and submitted a PR.
For more information on contributing to docs please visit https://docs.radapp.dev/contributing/docs/. |
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... |
7e1042e
to
0b73442
Compare
You are editing files which require a docs update. Please ensure you've made the appropriate changes to the docs and submitted a PR.
For more information on contributing to docs please visit https://docs.radapp.dev/contributing/docs/. |
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... |
0b73442
to
ecec4a9
Compare
You are editing files which require a docs update. Please ensure you've made the appropriate changes to the docs and submitted a PR.
For more information on contributing to docs please visit https://docs.radapp.dev/contributing/docs/. |
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... |
|
||
func (d *terraformDriver) fetchAzureCredentials() (*credentials.AzureCredential, error) { | ||
secretProvider := provider.NewSecretProvider(provider.SecretProviderOptions{ | ||
Provider: provider.TypeKubernetesSecret, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Provider: provider.TypeKubernetesSecret
-- this needs to be fixed to use whatever provider UCP is initialized to use.
desc string | ||
scope string | ||
expectedScopeSegments []ScopeSegment | ||
}{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any bugs or just realize we were missing test coverage?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just missing test coverage, making sure Parse
continues to work for scope now that Terraform implementation uses it :)
Description
Initial prototype that can enable us to catch any unknown blockers early on. Doesn't include unit tests and optimizations yet.
Issue reference
Fixes: #issue_number
Checklist
Please make sure you've completed the relevant tasks for this PR, out of the following list: