-
Notifications
You must be signed in to change notification settings - Fork 274
refactor: ecosystem helpers #3892
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
another-rex
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 great!
| 'Ubuntu': Ubuntu, | ||
| 'Wolfi': APK, | ||
|
|
||
| # Ecosystems known in the schema, but without implementations. |
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.
Hmm... is there a test we can add here that reads the ecosystems.json in the osv-schema repo and checks it against this.?
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 remember bringing this up a while ago - #2615 (comment)
Not sure if you agree/disagree with Andrew's reasoning here. With renovate automatically updating the osv-schema it probably would always fail the tests.
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.
That should be fine though, with the recent ecosystems, they made both PRs (osv-schema and osv.dev _ecosystem.py) simultaneously. So we just need to merge the ecosystems.py PR before updating the schema to the newest version and it should work right?
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.
We currently have a test that checks that all the ecosystems in _ecosystems.py are valid ecosystems in the schema. Adding the vice versa test (that all schema ecosystems are in _ecosystems.py) would mean we'd always have to update both at the same time.
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.
Ah I see..., let's leave it as it is then.
| self.assertEqual('1.20.0', ecosystem.next_version('a4', '1.18.0')) | ||
| with self.assertRaises(ecosystems.EnumerateError): | ||
| ecosystem.next_version('doesnotexist123456', '1') | ||
| with warnings.catch_warnings(): |
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.
did not know warnings was a thing, wow.
cuixq
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.
🎉
7cba064
Closes #3745
OrderedEcosystem(for ones that implement sort_key only) andEnumerableEcosystem(which has enumerate versions)Ecosystemclassnext_version(but marked it as deprecated) since only malicious packages uses it to convert some ghsa records.OrderingUnsupportedEcosystemsince it's no longer particularly useful.I also updated the copyright year on all the ecoystems py files :)
I think this also incidentally closes #3063, since
supports_comparingis no longer used (it will be superseded by #3850 anyway).