-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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 space and org names as tags to events #7297
Conversation
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 really good, I just have one suggestion for refactoring - left a comment inline.
params = { | ||
"results-per-page": MAX_PAGE_SIZE_V2, | ||
} | ||
headers = {"Authorization": "Bearer {}".format(self._oauth_token)} |
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.
It seems that this line is repeated multiple times. Maybe we can create a utility function get_auth_headers
that would refresh the get_oauth_token
and return this dict?
event_guid = "" | ||
event_ts = 0 | ||
|
||
if self._api_version == "v2": |
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.
It's a shame that you have to put back that code in the check itself. I like that you started hiding some of the v2/v3 differences. Ideally, all of it would be hidden in the check class, and we'd use some utilities. This is probably a bigger refactoring that can be done later though.
if not next_url: | ||
break | ||
page += 1 | ||
finally: |
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't you remove the try/finally and just do the service_check at the end?
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.
no because for events for instance, we break before the generator is exhausted, so we never reach the service check line in that case
Codecov Report
|
What does this PR do?
Fetch org and space names from Cloud Controller API to add as tags to events
Motivation
Customer request
Additional Notes
Review checklist (to be filled by reviewers)
changelog/
andintegration/
labels attached