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

[DPE-5866] Add Charm Terraform Module #501

Merged
merged 18 commits into from
Nov 22, 2024
Merged

[DPE-5866] Add Charm Terraform Module #501

merged 18 commits into from
Nov 22, 2024

Conversation

phvalguima
Copy link
Contributor

This PR extends our current charm to add a terraform module for the charm. It also adds github workflow to test + a basic test setup.

terraform/main.tf Outdated Show resolved Hide resolved
model = var.model_name
name = var.app_name
units = var.units
constraints = var.constraints
Copy link
Contributor

Choose a reason for hiding this comment

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

todo: From what I remember about OS, it's likely that we'll have multiple machines with different internal specs for different roles. High memory for ML, high disk for something else etc.

I think you might want to add an optional machines var here, that takes a list of juju_machine resources for placement.
I'm not 100% sure of the provider syntax for that, and it's changed a lot recently there and in the spec, but hopefully you get my gist.

Copy link
Contributor

Choose a reason for hiding this comment

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

Same with storages?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@marcoppenheimer, TL;DR: this is the "charm terraform". So, we are modeling a single "juju app" here. What you are referring to is the "product terraform". That may be composed by multiple types of juju apps, dashboard, etc. Indeed, for the later, we have to care about that.

Now, back to your questions, I agree with should have both as options: machines and storage.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@marcoppenheimer @ghislainbourgeois placement is broken in our provider. If I deploy without using placement, I still get a failure with:

╷                                                                                                                                  
│ Error: Provider produced inconsistent result after apply                                                                         
│                                                                                                                                  
│ When applying changes to module.opensearch.juju_application.opensearch, provider "provider[\"registry.terraform.io/juju/juju\"]" 
│ produced an unexpected new value: .placement: was cty.StringVal(""), but now cty.StringVal("1").                                 
│                                                                                                                                  
│ This is a bug in the provider, which should be reported in the provider's own issue tracker.                                     
╵                                                                                                                                  

There are bugs already reported:

juju/terraform-provider-juju#182 proposes a workaround with:

lifecycle {
        ignore_changes = [ placement, ]
    }

We should not skip changes in placement, nor constraints in my view.

Therefore, I am keeping placement out of this module for now (it will be commented only).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

BTW, I tried the ignore_changes and dis not work on my test. I am not sure if I missed something.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think keeping placement out for now is the right course of action until we find the right approach.

terraform/outputs.tf Outdated Show resolved Hide resolved
terraform/README.md Outdated Show resolved Hide resolved
Copy link
Collaborator

@ghislainbourgeois ghislainbourgeois left a comment

Choose a reason for hiding this comment

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

This looks great, nice job! On top of the comments that @marcoppenheimer already left, I added a comment regarding the variable model_name.

terraform/variables.tf Outdated Show resolved Hide resolved
@phvalguima
Copy link
Contributor Author

@ghislainbourgeois @marcoppenheimer I have closed the comments that seem to be addressed. There is one big question for me about placement. Can you check that out?

Copy link
Contributor

@Mehdi-Bendriss Mehdi-Bendriss left a comment

Choose a reason for hiding this comment

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

Thanks Pedro, it looks good. I have small questions

terraform/variables.tf Outdated Show resolved Hide resolved
terraform/variables.tf Outdated Show resolved Hide resolved
terraform/variables.tf Outdated Show resolved Hide resolved
.gitignore Outdated Show resolved Hide resolved
terraform/tests/preamble.tf Outdated Show resolved Hide resolved
terraform/tests/providers.tf Outdated Show resolved Hide resolved
terraform/main.tf Outdated Show resolved Hide resolved
terraform/main.tf Outdated Show resolved Hide resolved
@phvalguima phvalguima merged commit 1fa589c into 2/edge Nov 22, 2024
30 of 41 checks passed
@phvalguima phvalguima deleted the DPE-5866-terraform branch November 22, 2024 13:46
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.

5 participants