-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
Add support for named ports in autodiscover (hints & templates) #19398
Conversation
Signed-off-by: chrismark <[email protected]>
💔 Tests FailedExpand to view the summary
Build stats
Test stats 🧪
Test errorsExpand to view the tests failures
Steps errorsExpand to view the steps failures
Log outputExpand to view the last 100 lines of log output
|
Signed-off-by: chrismark <[email protected]>
Signed-off-by: chrismark <[email protected]>
Signed-off-by: chrismark <[email protected]>
Signed-off-by: chrismark <[email protected]>
Pinging @elastic/integrations-platforms (Team:Platforms) |
Signed-off-by: chrismark <[email protected]>
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.
Nice feature! But there are some small things that I think need to be addressed.
Signed-off-by: chrismark <[email protected]>
81d1d0d
to
bcd44f2
Compare
@jsoriano thanks for the review. Comments have been addressed, let me know what you think! |
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.
Looks good, added a question about the keystore fix and some nitpicking.
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.
LGTM once CI goes green. Thanks!
Signed-off-by: chrismark <[email protected]>
Signed-off-by: chrismark <[email protected]>
Signed-off-by: chrismark <[email protected]>
Signed-off-by: chrismark <[email protected]>
Signed-off-by: chrismark <[email protected]>
Signed-off-by: chrismark <[email protected]>
Signed-off-by: chrismark <[email protected]>
Signed-off-by: chrismark <[email protected]>
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.
looks good to me!
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.
LGTM, spotted an small typo in docs, and added a suggestion for tests, but it can be done later.
Signed-off-by: chrismark <[email protected]>
Thanks for the reviews @kaiyan-sheng and @jsoriano! I've added a couple of tests for config templates too. |
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.
Thanks for adding the extra tests!
}, | ||
}, | ||
expected: []*common.Config{configPorts}, | ||
}, |
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.
👍
…tic#19398) (cherry picked from commit 1b22373)
What does this PR do?
Adds support for named ports in hints based autodiscover.
When for instance we have a target container like:
then we can reference the name of the port with
co.elastic.metrics/hosts: '${data.host}:${data.ports.web}'
in the annotations.Why is it important?
To cover cases like #15796.
Manual Testing
Hints based Autodiscover
co.elastic.metrics/hosts: '${data.host}:8222'
andco.elastic.metrics/hosts: '${data.host}:${data.port}
Template based Autodiscover
co.elastic.metrics/hosts: '${data.host}:8222'
andco.elastic.metrics/hosts: '${data.host}:${data.port}
Closes #15796