-
Notifications
You must be signed in to change notification settings - Fork 353
refactor(Repository): Add RepositoryProvenance attribute
#10575
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
refactor(Repository): Add RepositoryProvenance attribute
#10575
Conversation
b608307 to
cdc634c
Compare
e01ca18 to
1107a05
Compare
Just a rebase onto 66.1.0.
plugins/reporters/ctrlx/src/funTest/kotlin/CtrlXAutomationReporterFunTest.kt
Outdated
Show resolved
Hide resolved
plugins/reporters/opossum/src/test/kotlin/OpossumReporterTest.kt
Outdated
Show resolved
Hide resolved
9ac6ba5 to
101b10c
Compare
Okay, according to the tests, I clearly missed something.
Unfortunately, I need to finish up for the week, so I will be back at this on Monday.
If you have any feedback or input on the failing tests in the meantime, it would be greatly appreciated.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@ ## main #10575 +/- ## ============================================ - Coverage 57.57% 57.54% -0.03% Complexity 1702 1702 ============================================ Files 346 346 Lines 12829 12833 +4 Branches 1212 1212 ============================================ - Hits 7386 7385 -1 - Misses 4978 4983 +5 Partials 465 465
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
If you have any feedback or input on the failing tests in the meantime, it would be greatly appreciated.
I think the general problem is that older ORT results that do not contain a RepositoryProvenance still need to be deserializable. You probably need to write a custom deserializer for that...
9090be5 to
cc83150
Compare
I think the general problem is that older ORT results that do not contain a
RepositoryProvenancestill need to be deserializable. You probably need to write a custom deserializer for that...
Thanks for the hint, adding the custom Deserializer fixed a lot of tests. Quite some progress today.
Working on fixing the other tests. Will get back to it tomorrow.
6c12714 to
a799cee
Compare
@sschuberth @mnonnenmacher
Not quite done yet, but feedback on the fixes for the tests so far would be awesome for tomorrow. 😃
Not quite done yet, but feedback on the fixes for the tests so far would be awesome for tomorrow. 😃
Sorry, I'll not be available tomorrow.
Not quite done yet, but feedback on the fixes for the tests so far would be awesome for tomorrow. 😃
Sorry, I'll not be available tomorrow.
Oh, my bad. I forgot. See you next week.
This comment was marked as outdated.
This comment was marked as outdated.
f2bdb8c to
5333234
Compare
d12811f to
601b9ee
Compare
@pepper-jk, please rebase to get rid of a test failure.
@pepper-jk, please rebase to get rid of a test failure.
I think the failing test is unrelated. The Expat package got a security update on September 16, and Conan is delivering the new package now.
601b9ee to
8615132
Compare
@pepper-jk, please rebase to get rid of a test failure.
I think the failing test is unrelated. The Expat package got a security update on September 16, and Conan is delivering the new package now.
That's exactly the reason why you should do the rebase 😉 We fixed that already in main.
@pepper-jk, please rebase to get rid of a test failure.
I think the failing test is unrelated. The Expat package got a security update on September 16, and Conan is delivering the new package now.
That's exactly the reason why you should do the rebase 😉 We fixed that already in
main.
I am already working on the rebase as we speak. I was just checking the tests first to be sure yesterdays changes did not mess anything up.
8615132 to
07b78f2
Compare
Rebased on 68.2.0 to check in on tests. WIll rebase on main after.
07b78f2 to
1230128
Compare
Rebased on current main now.
As discussed in the community meeting today, the Repository should contain normalized vcsInfo and resolved revisions.
This should solve the remaining two threads. So that is what I will be working on now.
62104e4 to
512ec6b
Compare
In order to support non-VCS input for ORT, `KnownProvenance`s need to be supported as input for `anaylzer` and `scanner`. The `Repository` data structure appears to be the best place to facilitate this change. This change focusses on adding a `RepositoryProvenance` attribute to `Repository`, which allows to keep previous behavior. For now it also exposes the raw `VcsInfo` of the `provenance` as its `vcs` attribute. Signed-off-by: Jens Keim <jens.keim@forvia.com>
Remove `vcs` and `vcsProcessed` attributes from `Repository`, since the `RepositoryProvenance` `provenance` is now the main attribute. Add `RepositoryDeserializer` to support parsing of legacy `OrtResult` files containing the previous attributes `vcs` and `vcsProcessed`, as well as the new format. It also needs to handle the `nestedRepositories` and `config` attributes for both legacy and recent `OrtResult` files. Signed-off-by: Jens Keim <jens.keim@forvia.com>
Signed-off-by: Jens Keim <jens.keim@forvia.com>
...blank Signed-off-by: Jens Keim <jens.keim@forvia.com>
Signed-off-by: Jens Keim <jens.keim@forvia.com>
Signed-off-by: Jens Keim <jens.keim@forvia.com>
Since these files are just parsed as plain text instead of deserialized into ORT data structures, updating them to the syntax appears to be the only way to fix the tests from here on out. Signed-off-by: Jens Keim <jens.keim@forvia.com>
Signed-off-by: Jens Keim <jens.keim@forvia.com>
`Repository.provenance.vcsInfo` would be even better, but KDoc appears to not be able to follow at this attribute depth. Signed-off-by: Jens Keim <jens.keim@forvia.com>
Signed-off-by: Jens Keim <jens.keim@forvia.com>
Signed-off-by: Jens Keim <jens.keim@forvia.com>
Signed-off-by: Jens Keim <jens.keim@forvia.com>
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 EvaluatedModelReporterFunTests are currently failing, since the Deserializer can not extract a resolvedRevison from the legacy Repository data structure, as the vcs_processed only references master. See reporter-test-input.yml, which is used as input in the tests.
I assume I would need to iterate over the other nodes in the file and get the resolved_revision from a provenance, e.g. inside the scanner at L542 or the list of provenances at L438.
Since there is a findParent method, I assume it would at least be possible to iterate over other nodes from inside a Deserializer, but I have not confirmed this yet.
@sschuberth @mnonnenmacher Before I continue on this hunch, could you way in on the issue?
baab4b2 to
c45f7ba
Compare
Rebased onto main aka 69.0.0.
Uh oh!
There was an error while loading. Please reload this page.
This draft includes the first changes for my personal POC for the
LocalProvenanceInput discussed in #8803.As it's scope is so small, it might be good first contribution towards that goal.
If not, feel free to let me know. All feedback is welcome.
The
Repositorydata structure seems the easiest place to allowProvenances asAnalyzerandScannerinput.This avoids refactoring the attributes of higher level data structures, such as
OrtResult.At the moment the implementation limits it to
RepositoryProvencethough, in order to keep its previous behavior.(削除) To that end, it also continues to expose the rawVcsInfoof theprovenanceas itsvcsattribute. (削除ここまで)Signed-off-by: Jens Keim jens.keim@forvia.com