-
Notifications
You must be signed in to change notification settings - Fork 4
Added settings to enable sending new structures and updates to instru… #51
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
Conversation
…ment capabilities to HEROIC
cmccully
left a comment
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.
In general this looks fine to me. I'd like another review from someone who is more familiar with configdb though.
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.
Pull Request Overview
This PR integrates HEROIC reporting for instrument capabilities by adding settings, signal handlers, helper functions, and hooks in serializers/admin to trigger updates, along with tests to verify the behavior.
- Introduces HEROIC API environment settings and AppConfig conditional loading
- Implements
configdb.hardware.heroicmodule with helper functions andsend_to_heroic - Updates serializers and admin classes to call
update_heroic_instrument_capabilitieson relevant CRUD operations - Adds comprehensive API tests for HEROIC update triggers
Reviewed Changes
Copilot reviewed 7 out of 8 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| configdb/settings.py | Added HEROIC API URL, token, and observatory env settings |
| configdb/hardware/tests.py | New TestHeroicUpdates class with mocked send_to_heroic tests |
| configdb/hardware/signals/handlers.py | Signal handlers to create/update HEROIC entities |
| configdb/hardware/serializers.py | Serializer update methods enhanced to trigger HEROIC updates |
| configdb/hardware/heroic.py | New HELPER functions for formatting and sending to HEROIC API |
| configdb/hardware/apps.py | can_submit_to_heroic and conditional signal registration |
| configdb/hardware/admin.py | Admin save_related/save_model overrides to trigger updates |
Comments suppressed due to low confidence (2)
configdb/hardware/tests.py:165
- Add a test to verify that when an instrument's state is set to DISABLED,
send_to_heroicis not called.
def test_update_instrument_state_calls_out_to_heroic(self, mock_send):
configdb/settings.py:167
- [nitpick] Consider restricting CORS origins instead of allowing all (
CORS_ORIGIN_ALLOW_ALL = True) to reduce exposure to unintended clients.
CORS_ORIGIN_ALLOW_ALL = True
…ment capabilities to HEROIC
I originally thought I would do this entirely with django signals (pre_save for each model), but unfortunately those signals are not triggered with new data when all of the many ManyToMany relationships in models are updated. In order to ensure we only have a single instrument capability update sent to HEROIC per update made in the admin interface or API, I moved the main HEROIC reporting to occur in those two places.
So for LCO's usage, where we update everything in the Admin interface generally, the extra code in the admin.py file will trigger HEROIC reporting when any instrument capability change is made in that interface. Alternatively, in the serializers.py, HEROIC reporting will be triggered by API updates of various parts of the instrument capabilities. The few signal handlers remaining don't involve any M2M relationships so they should work for both API/Admin interface cases and are just to create the instrument/site/telescope and update their properties. I've unit tested the API usage to see that it calls the code to send the update to HEROIC when updates are made through the API. I've manually tested the Admin interface locally as well.