-
-
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
Report validation error if resources not specified #1107
Report validation error if resources not specified #1107
Conversation
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:pr1107-linux Want to verify the new version? Run it locally: docker run -d -p 8999:80 --name promitor-agent-scraper-pr1107-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:pr1107-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, thanks!
Would you mind adding an entry on https://github.com/tomkerkhove/promitor/blob/master/changelog/content/experimental/unreleased.md please?
@tomkerkhove I'm happy to do that - I'm just not sure what the best thing to do is because you've already added the following entry to the changelog:
I'm thinking it might be confusing if we put it in another entry that's pretty similar. I realise your entry is for the internal model validation, but what I've added is for the configuration validation, but maybe the distinction won't be important for users? I'll go ahead and add changelog entries for my other two PRs anyhow - just need some guidance on this one. |
That's a valid point, no need to take any action then! Once the code quality check is passing we can merge this one, thanks! |
542977d
to
0c55738
Compare
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:pr1107-linux Want to verify the new version? Run it locally: docker run -d -p 8999:80 --name promitor-agent-scraper-pr1107-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:pr1107-linux You can find a CI version of our Helm chart on hub.helm.sh |
I've updated the MetricDefinitionDeserializer to report a validation error if neither `resources` or `resourceCollections` is populated. Fixes tomkerkhove#806
0c55738
to
6c05a9f
Compare
Ok - should be sorted now @tomkerkhove |
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:pr1107-linux Want to verify the new version? Run it locally: docker run -d -p 8999:80 --name promitor-agent-scraper-pr1107-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:pr1107-linux You can find a CI version of our Helm chart on hub.helm.sh |
Thanks man! |
I've updated the MetricDefinitionDeserializer to report a validation error if neither
resources
norresourceCollections
is populated.Fixes #806
A metric definition like the following:
Will now produce the following validation error: