Redis enterprise - added dash to name of overview dashboard

j8-redis commented 3 months ago

What does this PR do?

A brief description of the change being made with this pull request.

Motivation

What inspired you to submit this pull request?

Review checklist

[ ] PR has a meaningful title or PR has the no-changelog label attached
[ ] Feature or bugfix has tests
[ ] Git history is clean
[ ] If PR impacts documentation, docs team has been notified or an issue has been opened on the documentation repo
[ ] If this PR includes a log pipeline, please add a description describing the remappers and processors.

Additional Notes

Anything else we should know when reviewing?

j8-redis commented 3 months ago

A recent pull request ran into a few snags, one of which is the following:

pytest.raises(Exception) should be considered evil`

This is the code in question:

with pytest.raises(Exception):
try:
    dd_run_check(check)

I spent some time reviewing the message itself as well as the pytest documentation and I didn't find anything wrong with the basic pattern, so it has to be the use of 'Exception'.

However, when a datadog_checks.base.errors.Configuration error is reported it reports itself as Exception, and I confess I do not know why. The following code works, but if I set the else assertion to False, as it indeed should be, it fails. And it will also fail if I ask only for a ConfigurationError. Can you have someone in Engineering have a look?

try:
    dd_run_check(check)
except Exception as ex:
    if ex.__class__ == ConfigurationError.__class__:
        assert True
    else:
        assert True

This also works properly if I raise a ConfigurationError, eg.

with pytest.raises(ConfigurationError):
    raise ConfigurationError('')

Perhaps the original is being caught and then re-raised as a base-level Exception?

j8-redis commented 3 months ago

HI I've reverted the CODEOWNERS commit and now it's showing up as a failure. Should I assume that I can ignore this failure as well as the lint issue? I think I have a fix for the lint issue but it's ugly and not good practice.

bgoldberg122 commented 3 months ago

HI I've reverted the CODEOWNERS commit and now it's showing up as a failure. Should I assume that I can ignore this failure as well as the lint issue? I think I have a fix for the lint issue but it's ugly and not good practice.

@j8-redis Yes, you can ignore the CODEOWNERS failure. I think the lint issue needs to be fixed though -- @iliakur can you please confirm?

iliakur commented 3 months ago

HI I've reverted the CODEOWNERS commit and now it's showing up as a failure. Should I assume that I can ignore this failure as well as the lint issue? I think I have a fix for the lint issue but it's ugly and not good practice.

@j8-redis Yes, you can ignore the CODEOWNERS failure. I think the lint issue needs to be fixed though -- @iliakur can you please confirm?

@j8-redis @bgoldberg122 the validation is in fact correct. The logs-backend team wants to co-own all logs pipelines in integrations. So you should get the validation to pass by adding them back to Codeowners please.

bgoldberg122 commented 3 months ago

HI I've reverted the CODEOWNERS commit and now it's showing up as a failure. Should I assume that I can ignore this failure as well as the lint issue? I think I have a fix for the lint issue but it's ugly and not good practice.

@j8-redis Yes, you can ignore the CODEOWNERS failure. I think the lint issue needs to be fixed though -- @iliakur can you please confirm?

@j8-redis @bgoldberg122 the validation is in fact correct. The logs-backend team wants to co-own all logs pipelines in integrations. So you should get the validation to pass by adding them back to Codeowners please.

@iliakur The codeowners error is not for this integration, so I will fix it in a separate PR

DataDog / integrations-extras