-
Notifications
You must be signed in to change notification settings - Fork 39
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
Implement an sw/hw/fw upgrade event handler plugin #2515
Implement an sw/hw/fw upgrade event handler plugin #2515
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2515 +/- ##
==========================================
- Coverage 53.54% 52.59% -0.96%
==========================================
Files 556 556
Lines 40432 39615 -817
==========================================
- Hits 21651 20834 -817
Misses 18781 18781
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
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 great, but I have one question about the available variables - and:
The diff coverage is less than 50%, so codecov complains. Can you get this under test so we keep our coverage stats up?
Are there any tests for eventengine plugins that I could orient myself on? |
Sure. There is |
Kudos, SonarCloud Quality Gate passed! 0 Bugs No Coverage information |
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.
Seeing the changes you implemented as a result of my previous review, I see that my feedback could have been a bit more specific regarding the old/new version variable stuff. Looks to me like you need an extra PR 😛
Also, codecov still doesn't seem very happy 😞
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.
So, after an out-of-band discussion about missing code coverage, the conclusion is thus:
There is no need/reason for this plugin to be tested in a black-box fashion, involving a full eventengine daemon run, as the upgrade plugin's behavior is quite simple.
You're better off testing the plugin class directly. This doesn't start a subprocess, where the collection of coverage data can be problematic.
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.
Much better, just nitpicks left :)
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.
🎉
852c632
to
196c736
Compare
Copy old/new version to alert history to use in reports
196c736
to
3555eb2
Compare
Kudos, SonarCloud Quality Gate passed! 0 Bugs No Coverage information |
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.
🎉 🎉 🎉
The plugin copies the variables
old_version
andnew_version
(if exists) to alert history to use in reports (#2466)I decided to not just do it for software upgrades, but also firmware and hardware in case we also want to create reports for that.
Also fixes #2533 as a side effect.