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.
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
[DPE-5866] Add Charm Terraform Module #501
Changes from all commits
ee86920
6f74cde
691fc8b
ec2f8e5
113dd5e
4b4a292
bdfce92
a0ae40a
adf10b8
b4891c3
3c17d57
bab616e
cd1a355
9ed7cdf
279b62a
7237e03
dd67416
0853d0c
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
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 ofjuju_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.
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.
Same with storages?
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.
@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
andstorage
.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.
@marcoppenheimer @ghislainbourgeois
placement
is broken in our provider. If I deploy without using placement, I still get a failure with:There are bugs already reported:
juju/terraform-provider-juju#182 proposes a workaround with:
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).
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.
BTW, I tried the ignore_changes and dis not work on my test. I am not sure if I missed something.
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 think keeping placement out for now is the right course of action until we find the right approach.