-
Notifications
You must be signed in to change notification settings - Fork 7
Shared actions 🔥 #5
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
base: main
Are you sure you want to change the base?
Conversation
mikejgray
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.
Great additions! I have some recommendations to help future-proof these a bit
mikejgray
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.
Outstanding work! A couple of questions as I'm not sure how the edge cases would be handled, but otherwise LGTM
builderjer
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.
If these are to be common usage, and required additions to an "official package" skill or otherwise, I think a good README.md of how to use these would be justified.
|
What's the deal with the existing README text anyway? Doesn't really fit in |
Feel free to adjust to what fits in. I believe that was once just cooy/pasted it when the repo got created. |
|
Awesome 👍 Good job @emphasize! |
mikejgray
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.
Small nit, but nothing that would hold up an approval. Nicely done!
NeonDaniel
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.
I haven't been able to test all of the automations, but left notes where test cases may be importable from neon-minerva to consolidate things/prevent diverging changes
| return skill | ||
|
|
||
|
|
||
| class TestSkillIntentMatching(unittest.TestCase): |
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.
This appears to be a functionally equivalent to neon-minerva. consider install/import to reduce duplicated code
| @@ -0,0 +1,200 @@ | |||
| # NEON AI (TM) SOFTWARE, Software Development Kit & Application Framework | |||
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.
Direct copy of neon-minerva test class, consider installing/importing rather than duplicating for future maintenance
this would make us unable to test anything pinning a higher version that minerva allows, in general it is expected that minerva will be slower to adopt anything we do in ovos due to Neon being downstream, shouldnt be an issue right now, but i think it will be an issue sooner or later, its one major ovos-utils or ovos-workshop update away I also expect the code to diverge, as a lot of minerva will be Neon specific and not apply to us necessarily, or make assumptions that only apply to Neon, scope of the package will be very different even if some tests are shared This isnt really a problem now, but something to consider, i prefer to have any essential infra under the OVOS org if all ovos packages are going to literally depend on this We can ofc change things at any point if incompatibilities arise, so this isnt necessarily a blocker if everyone else thinks it best to import minerva, i feel we are better future proofed this way but dont particularly care either way right now |
|
let's get this one merged and follow up with companion tweaks in separate PRs as needed :) this is awesome work by @emphasize , a bunch of our repos have no automations, we can start by testing the new stuff live on those right away to iron out any quirks in the process |
We should have at least some validation of every added automation; I would suggest we break this up into smaller PRs so we can validate one or a few things at a time. I'll also note that some of these appear to be duplicates from https://github.com/NeonGeckoCom/.github/tree/master/.github/workflows which are already in use in OVOS projects and it would be less work to contrib changes there than updating all of the existing workflows |
Neon has always made an effort to support OVOS skills/plugins explicitly and I'm currently working on adopting the base classes from ovos-workshop directly. I'm not going to argue that these have to use minerva, but it would reduce a decent amount of duplicated code and consolidate efforts. Keeping it in minerva also allows use outside of GHA since minerva exposes a CLI for running tests locally. |
mikejgray
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.
This is a good foundation to start from but still needs a bit of work to get it ready for release, IMHO. Would recommend either prioritizing that effort or closing this PR.
No more fumbling around on a per repo basis.