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

Added support for group alert messages #890

Merged
merged 3 commits into from
Jul 9, 2023
Merged

Conversation

hickey
Copy link
Contributor

@hickey hickey commented Jul 5, 2023

These changes are to address the feature request that I put forward in #872.

At this point the changes are untested, but they appear that they would work. If it is possible to build an AREDN firmware from this branch, then I would be very happy to test the changes on one of my nodes to validate that the functionality is working.

Signed-off-by: Gerard Hickey <hickey@kinetic-compute.com>
@ab7pa
Copy link
Contributor

ab7pa commented Jul 5, 2023

I have two suggestions. First, make the Alert Groups feature part of the "AREDN Alert Settings" category so they appear along with all of the other Alert Settings. Second, the grouping feature is actually a subset of Local Messages so they should be under that code section rather than appending them separately at the end. I'm attaching two files here: one shows the Alert Groups as part of the AREDN Alert Settings category, and the second shows what the entire aredn_message.sh script could look like with the groups as part of the Local Message section of code. I have tested both of these attached files on my Alert node and they work as you envisioned.

advancedconfig-alertgroup.txt

aredn_message.sh.txt

@ab7pa
Copy link
Contributor

ab7pa commented Jul 5, 2023

Here is what the Alert Group code I suggested above looks like on one of my alert subscriber nodes:

2023-07-05_09-39

@hickey
Copy link
Contributor Author

hickey commented Jul 5, 2023

Ah. I was not looking at the fields close enough. The category field I was thinking that it was a field name. The intention was to keep the setting with the other alert fields. I will get the category field updated to get it included in the alert section.

Your screen capture off one of your nodes is precisely what I was envisioning also.

Are there any GitHub actions out there that I can add to my fork to build the sysimage file so that I can install and test changes to insure that things are working? Actually with these changes the amount of updates are easy and I can go update one of my nodes (probably similar to what you did). It would be nice to have a way to generate the sysimage from a branch so that bigger changes can be tested easily.

Signed-off-by: Gerard Hickey <hickey@kinetic-compute.com>
@hickey
Copy link
Contributor Author

hickey commented Jul 6, 2023

I have updated advancedconfig and I am still working on aredn_message.sh. I have refactored out all the wget commands into a function since it is all the same operation and makes the code a bit more readable. I am looking at writing the final alert text to the file at same time as that would clean up a bit more code too.

I have created aredn/documentation#273 for documenting this feature.

Signed-off-by: Gerard Hickey <hickey@kinetic-compute.com>
@hickey
Copy link
Contributor Author

hickey commented Jul 6, 2023

@ab7pa I have completed the updates to aredn_message.sh. Take a look and see if you see any issues. I have tested the local and group alerts and can not fore see any issues, but I am unable to test the AREDN messages. Ideally they should be handled in the same manner as before.

Copy link
Contributor

@ab7pa ab7pa left a comment

Choose a reason for hiding this comment

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

Looks good, @hickey And thanks for the corresponding documentation update too.

@aanon4 aanon4 self-requested a review July 9, 2023 05:05
Copy link
Contributor

@aanon4 aanon4 left a comment

Choose a reason for hiding this comment

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

Good to go.

@aanon4 aanon4 merged commit f4321ff into aredn:main Jul 9, 2023
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.

None yet

3 participants