-
-
Notifications
You must be signed in to change notification settings - Fork 92
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
Provide validation rule to verify if resource discovery is configured when metric discovery is used #1142
Conversation
Signed-off-by: Tom Kerkhove <kerkhove.tom@gmail.com>
Docker image for this PR was built and is available on Docker Hub. You can pull it locally via the CLI: docker pull tomkerkhove/promitor-agent-scraper-ci:pr1142-linux Want to verify the new version? Run it locally: docker run -d -p 8999:80 --name promitor-agent-scraper-pr1142-linux \
--env PROMITOR_AUTH_APPID='<azure-ad-app-id>' \
--env-file C:/Promitor/az-mon-auth.creds \
--volume C:/Promitor/metrics-declaration.yaml:/config/metrics-declaration.yaml \
--volume C:/Promitor/runtime-config.yaml:/config/runtime.yaml \
tomkerkhove/promitor-agent-scraper-ci:pr1142-linux You can find a CI version of our Helm chart on hub.helm.sh |
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 - just made a minor suggestion about the wording of the error message.
if (_configuration == null) | ||
{ | ||
if (doesDeclareResourceDiscoveryGroups) | ||
{ | ||
return ValidationResult.Failure(ComponentName, new List<string> {"No resource discovery is configured while discovery groups are defined to scrape"}); |
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 I maybe suggest rewording this to something like:
Resource discovery groups are defined in your metrics configuration, but resource discovery has not been configured. Please add a resource discovery configuration.
I just feel like it's a bit more explicit then, and it also points people towards a solution.
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.
Sure, good suggestion!
Docker image for this PR was built and is available on Docker Hub. You can pull it locally via the CLI: docker pull tomkerkhove/promitor-agent-scraper-ci:pr1142-linux Want to verify the new version? Run it locally: docker run -d -p 8999:80 --name promitor-agent-scraper-pr1142-linux \
--env PROMITOR_AUTH_APPID='<azure-ad-app-id>' \
--env-file C:/Promitor/az-mon-auth.creds \
--volume C:/Promitor/metrics-declaration.yaml:/config/metrics-declaration.yaml \
--volume C:/Promitor/runtime-config.yaml:/config/runtime.yaml \
tomkerkhove/promitor-agent-scraper-ci:pr1142-linux You can find a CI version of our Helm chart on hub.helm.sh |
Signed-off-by: Tom Kerkhove kerkhove.tom@gmail.com
Closes #1069