-
Notifications
You must be signed in to change notification settings - Fork 6
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
Date filter, logging and types #26
Date filter, logging and types #26
Conversation
Signed-off-by: Shashank Reddy Boyapally <[email protected]>
Signed-off-by: Shashank Reddy Boyapally <[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.
Can we add a lookback test?
speaking of, I see there are unit tests in a workflow. But I don't see it running on this pr. Is the workflow run part properly configured? |
Unit tests are performed on push, in the fork branch as of now, we can configure it to run for pull requests too. |
@shashank-boyapally do you see the test results on your fork then? I think it would be more helpful to show that result in the PR itself so we can know if everything is passing before merge |
Signed-off-by: Shashank Reddy Boyapally <[email protected]>
Signed-off-by: Shashank Reddy Boyapally <[email protected]>
Signed-off-by: Shashank Reddy Boyapally <[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! lgtm! Thanks!
Signed-off-by: Shashank Reddy Boyapally <[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.
approving again ;)
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
meta["networkType"] = "OVNKubernetes" | ||
meta["benchmark.keyword"] = "cluster-density-v2" | ||
# meta['encrypted'] = "true" | ||
# meta['ipsec'] = "false" | ||
# meta['fips'] = "false" | ||
|
||
uuids = match.get_uuid_by_metadata(meta) | ||
print("All uuids",len(uuids)) |
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.
Are these just debugs?
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.
yep this is just a utility for functionality testing.
import sys | ||
|
||
|
||
class SingletonLogger: |
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.
I don't think we need a singleton class here. It just as simple as updating a config.
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.
Thank you for the suggestion. The thought behind this was as follows: having a logger setup in Matcher class created duplicate logs of the same log-output, especially in daemon mode where multiple Matcher objects would be created as when the request would come in and multiple formatters were being attached to the logger which led to duplicate logs. To mitigate this a temporary solution was to check if the logger existed previously. I transferred this logic to a class so that it can be used in Orion as well. The SingletonLogger here also updates the config using methods and also keeps track of only having a single logger.
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.
logging is singleton by default. All I am saying is, I think we need not write a custom logging class instead of just using the existing ones.
"ignore", category=UserWarning, message=".*Connecting to.*verify_certs=False.*" | ||
) | ||
|
||
match = Matcher(index="perf_scale_ci*", verify_certs=False) |
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.
Do we want to change it to ospst-*
as well?
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.
Currently this utility works on the qe instance...
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.
Nope - as that would require having access to internal resources.
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.
okay. Understood.
if len(uuids) == 0: | ||
print("No UUID present for given metadata") | ||
sys.exit() | ||
runs = match.match_kube_burner(uuids) | ||
runs = match.match_kube_burner(uuids,"ripsaw-kube-burner*") |
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.
Same here as well ospst-*
Type of change
Description
Added logging for common usage, date filter for orion's new feature of lookback and types to methods
Related Tickets & Documents
Checklist before requesting a review
Testing