-
Notifications
You must be signed in to change notification settings - Fork 7
feat: support logs-agent resources configuration #470
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
base: main
Are you sure you want to change the base?
feat: support logs-agent resources configuration #470
Conversation
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 1 comment from me. @Aashiq-J as codeowner you any other concerns?
18540f5
to
84309df
Compare
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 looks good to me.
84309df
to
2d78c4c
Compare
/run pipeline |
a8a6f02
to
85a4517
Compare
/run pipeline |
Oh the tests are blocked due to this issue: IBM-Cloud/terraform-provider-ibm#6036 |
Provider patch release coming today - will re-run after its released |
/run pipeline |
1 similar comment
/run pipeline |
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.
@fidel-ibm Your going to have to do some testing to get this to work. The tests failed with:
│ Error: failed parsing key "resources" with value {"limits":{"cpu":"500m","ephemeral_storage":"10Gi","memory":"3Gi"},"requests":{"cpu":"100m","ephemeral_storage":"2Gi","memory":"1Gi"}}, key "\"requests\":{\"cpu\":\"100m\"" has no value (cannot end with ,)
│
│ with module.observability_agents.module.logs_agent[0].helm_release.logs_agent,
│ on ../../modules/logs-agent/main.tf line 40, in resource "helm_release" "logs_agent":
│ 40: resource "helm_release" "logs_agent" {
│
╵}
modules/logs-agent/main.tf
Outdated
@@ -112,6 +112,11 @@ resource "helm_release" "logs_agent" { | |||
value = var.logs_agent_enable_scc | |||
} | |||
|
|||
set { | |||
name = "resources" | |||
value = jsonencode(var.logs_agent_resources) |
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.
maybe you need to use join("\\,"
like what was done in other sets for object types
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.
hmm not sure what the issue is in this case, I cannot use join
because that expect a list and we are passing an object.
I've moved this code to inside the values definition, to see if the yamlencode function handles the transformation well.
When I run terraform plan even with the previous setup, it had the proper output for the config.
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.
@fidel-ibm see comments
d5eddd8
to
6968fac
Compare
6968fac
to
7b0c654
Compare
a819b41
to
43eafd5
Compare
/run pipeline |
…thub.com:fidel-ibm/terraform-ibm-observability-agents into fidel-ruiz/feat-add-support-logs-agent-resources
43eafd5
to
3a47f2f
Compare
/run pipeline |
@fidel-ibm Any joy locally testing this? The tests are failing with:
|
@jor2 I see you added support in terraform-ibm-modules/terraform-ibm-logs-agent#2 - Does it mean you got it working? Can you leave a comment in this PR as to what the issue is? |
It works when you remove the field |
Description
Allows configuration of resources for the logs-agent.
Git issue: #469
Release required?
x.x.X
)x.X.x
)X.x.x
)Release notes content
Run the pipeline
If the CI pipeline doesn't run when you create the PR, the PR requires a user with GitHub collaborators access to run the pipeline.
Run the CI pipeline when the PR is ready for review and you expect tests to pass. Add a comment to the PR with the following text:
Checklist for reviewers
For mergers