-
Notifications
You must be signed in to change notification settings - Fork 0
feat: move logs agents to its own tile #2
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?
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.
Left a few comments, but maybe this is still WIP as I notice the DA inputs still have cloud monitoring inputs there?
a43364d
to
9a924ef
Compare
@jor2 Can you add same codeowners as https://github.com/terraform-ibm-modules/terraform-ibm-observability-agents/blob/main/.github/CODEOWNERS |
62b2320
to
c93af1d
Compare
/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.
- See comments
- The diagram should show some kind of flow of the agents sending data to a Cloud Logs instance.
- Can we please use cross variable validation in the DA inputs
README.md
Outdated
Use real values instead of "var.<var_name>" or other placeholder values | ||
unless real values don't help users know what to change. | ||
--> | ||
## Usage |
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 you change back the readme so it aligns with the module template please
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.
@jor2 This is still an open comment
} | ||
|
||
variable "logs_agent_additional_metadata" { | ||
description = "The list of additional metadata fields to add to the routed logs." |
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 a complex input so needs a corresponding markdown helper doc
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.
still open comment
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.
@jor2 Ca you see if you can add support for terraform-ibm-modules/terraform-ibm-observability-agents#470 in this PR too? I think the logic is slightly incorrect in that PR so you might need to fix it to get it working |
/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.
- cloud logs icon is missing from diagram
- You dont need to commit
images/cloud-logs-icon.svg
? - Can you add this to the renovate.json?
{
"$schema": "https://docs.renovatebot.com/renovate-schema.json",
"extends": ["github>terraform-ibm-modules/common-dev-assets:commonRenovateConfig"],
"customManagers": [
{
"customType": "regex",
"description": "Update agent version to the latest in variables.tf",
"fileMatch": ["variables.tf$"],
"datasourceTemplate": "docker",
"matchStrings": [
"default\\s*=\\s*\"(?<currentValue>.*)\"\\s*# datasource: (?<depName>[^\\s]+)"
]
}
]
}
} | ||
|
||
variable "logs_agent_additional_metadata" { | ||
description = "The list of additional metadata fields to add to the routed logs." |
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.
still open comment
/run pipeline |
1 similar comment
/run pipeline |
/run pipeline |
/run pipeline |
error - known issue. https://ibm-cloudplatform.slack.com/archives/C02DYMP47EF/p1743596567371199?thread_ts=1743522645.890589&cid=C02DYMP47EF
|
/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.
@jor2 see comments
README.md
Outdated
Use real values instead of "var.<var_name>" or other placeholder values | ||
unless real values don't help users know what to change. | ||
--> | ||
## Usage |
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.
@jor2 This is still an open comment
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.
@jor2 The chart and image versions need to be separate inputs. So we should have:
- logs_agent_image
- logs_agent_image_version
- logs_agent_chart
- logs_agent_chart_location
- logs_agent_chart_version
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.
see comments
Description
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