Skip to content
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

SQL Database Compatibility #84

Merged
merged 15 commits into from
Dec 15, 2023
Merged

SQL Database Compatibility #84

merged 15 commits into from
Dec 15, 2023

Conversation

KaraMelih
Copy link
Collaborator

In the past we got rid of the meta field where we used to group all unknown key-value pairs. Instead we were now adding them as new key-value pairs to the dictionary.

However, with the new SQL backed, this seemed more complicated to track. So, I rolled back to the original idea.

If the user builds a message that contains some key-value pairs "that do not belong to ANY of the created tiers" they are shipped under the "meta" key in the message. These meta keys are NOT displayed in the alert messages but they are there for long-term storage and additional investigation, e.g. any comment the user might have, or any additional data that they might provide for triangulation etc. Coincidence System (snews cs) does not try to investigate the data send under the meta.

Along with this addition, I also

  • fix the detector name setting issue
  • made is_test an accepted argument for each tier, defaulted it to False when nothing is passed
  • updated the notebooks

@KaraMelih KaraMelih marked this pull request as draft December 11, 2023 17:18
@KaraMelih
Copy link
Collaborator Author

Oops. The test scripts need to be adjusted. Converting this to a draft pull request.

@KaraMelih KaraMelih marked this pull request as ready for review December 12, 2023 10:42
Copy link
Collaborator Author

@KaraMelih KaraMelih left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I went over the changes and it looks correct to me. The tests are also passing. Nevertheless, maybe it is better if a fresh set of eyes take a look and merge this PR

@KaraMelih KaraMelih merged commit 6c3bbd1 into main Dec 15, 2023
2 checks passed
KaraMelih added a commit that referenced this pull request Dec 19, 2023
Merge pull request #84 from SNEWS2/patch
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants