-
Notifications
You must be signed in to change notification settings - Fork 28
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 signal for Owner #940
add signal for Owner #940
Conversation
This PR includes changes to |
4703f30
to
04c2474
Compare
codecov_auth/signals.py
Outdated
"service", | ||
] | ||
if created or any(instance.tracker.has_changed(field) for field in tracked_fields): | ||
pubsub_project_id = settings.SHELTER_PUBSUB_PROJECT_ID |
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.
Optional: since this is used in a couple of places, would encourage to move this into its own fn and use that in every signal for readability!
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 agree - I added one, check it out!
Also of course, please double check shared changes are good to go. And to update to the Shared sha after it merges |
Codecov ReportAll modified and coverable lines are covered by tests ✅
✅ All tests successful. No failed tests found. Additional details and impacted files@@ Coverage Diff @@
## main #940 +/- ##
==========================================
+ Coverage 96.23% 96.24% +0.01%
==========================================
Files 825 826 +1
Lines 19027 19028 +1
==========================================
+ Hits 18311 18314 +3
+ Misses 716 714 -2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Codecov ReportAll modified and coverable lines are covered by tests ✅ ✅ All tests successful. No failed tests found. 📢 Thoughts on this report? Let us know! |
❌ 321 Tests Failed:
View the top 3 failed tests by shortest run time
To view individual test run time comparison to the main branch, go to the Test Analytics Dashboard |
28657a5
to
c00b865
Compare
codecov_auth/signals.py
Outdated
if not _pubsub_publisher: | ||
_pubsub_publisher = pubsub_v1.PublisherClient() | ||
return _pubsub_publisher | ||
def publish(self, data: Dict[str, Any]) -> None: |
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! This looks great! A last nitpick, could we move this to somewhere like utils/shelter.py
in the root folder, or anywhere you see fit? This isn't strictly a codecov_auth responsibility so it makes sense it is its own standalone thing
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.
agree! moved!
88ca9ac
to
bb763b5
Compare
Purpose/Motivation
Shelter
needs to know some things aboutOwner
.Links to relevant tickets
codecov/engineering-team#2299
Goes with this piece https://github.com/codecov/shelter/pull/203
Depends on codecov/shared#411
What does this PR do?
This new signal will trigger
Shelter
to pick up the object and store the relevant fields.