Move resolvedRevision from VcsInfo to Provenance #4132

mnonnenmacher · 2021-06-08T20:32:36Z

This PR mainly moves the resolvedRevision property from VcsInfo to RepositoryProvenance to better model that this property is only supposed to be set by the downloader. Please see the commit messages for details.

mnonnenmacher · 2021-06-09T16:03:12Z

@sschuberth @fviernau This can already be reviewed, even though I expect that some tests might fail due to the last two commits I added. The tests already worked for the main commit which moves the property. This change is basically the outcome of the discussion we had a while ago, I hope it matches everyone's requirements. The only thing I didn't do is to add a property for symbolic names like branches or tags, but I think this can be done later if required.

sschuberth · 2021-06-09T16:07:36Z

This change is basically the outcome of the discussion we had a while ago, I hope it matches everyone's requirements. The only thing I didn't do is to add a property for symbolic names like branches or tags, but I think this can be done later if required.

I have to admit I was slightly confused by the PR's title, as I though the outcome of our discussion was rather to change the docs than the implementation of VcsInfo. But I'll first have a look before I complain 😉

mnonnenmacher · 2021-06-09T16:11:29Z

Yes, I mainly captured the "spirit" of our discussion, I have tried some approaches and found that this produced the cleanest code.

sschuberth

Will continue the review tomorrow.

sschuberth · 2021-06-09T16:37:02Z

analyzer/src/funTest/kotlin/integration/DrupalIntegrationFunTest.kt

-            "https://github.com/drupal/drupal.git",
-            "4a765491d80d1bcb11e542ffafccf10aef05b853",
-            ""
+            type = VcsType.GIT,


The resolvedRevision property was supposed to be only set by the downloader.

As mentioned in the past, I was believing this statement to be wrong; I was under the assumption that just the docs of that property were wrong, and that any part of ORT was free to put a resolved version of revision into resolvedRevision.

So I looked up when the property was introduced, which was in c87b658 (actually by you, Martin), and in the commit message you stated "The field is used to store the revision resolved from the working tree of a locally cloned repository in". So you're right and I was wrong 😁 Would you mind adding that commit's SHA1 as a reference so I remember?

The requirement for resolvedRevision previouly lead to problems

Which "requirement"? The property was optional after all. Do you mean "The presence of"?"

s/previouly/previously/

Address this by adding the new property isResolvedRevision to VcsInfo.

Hmm. Just thinking out aloud:

This means we cannot currently store the (symbolic) revision and its corresponding resolved revision at the same time , e.g. if provided by a package manager.

It also means we need to know if a revision is resolved or not. Previously, this was no requirement for the revision properly; it could have been resolved or not. Only for resolvedRevision there was a guarantee for it to be resolved.

Would you mind adding that commit's SHA1 as a reference so I remember?

Done.

Which "requirement"?

This meant the requirement described in the paragraph above, I have rephrased the sentence to make it clearer.

s/previouly/previously/

Done.

This means we cannot currently store the (symbolic) revision and its corresponding resolved revision at the same time , e.g. if provided by a package manager.

Yes, just like before. But now there is a way for package managers to show that a revision is resolved which wasn't possible before. I have only done it for NuGet in this PR because it was doing this before and I wasn't sure which other package managers support that, probably GoMod and NPM/Gradle with VCS dependencies.

As mentioned above I think if we want this to be possible it can be done separately. I didn't want to do this now because I mainly wanted to improve the current situation, not add new functionality. Also I don't see that much value in capturing a branch/tag name if we already have a specific revision. I could be used as a fallback if the revision doesn't work or be shown in the reports (with explaining that it could probably have nothing to do with the actually scanned revision, depending on where the data is coming from), but it doesn't seem that any of the package managers I work most with supports two properties for the revision anyway.

It also means we need to know if a revision is resolved or not.

No, we don't. If we set the flag to false even though the revision was resolved by the package manager nothing bad happens, except that the downloader might try to guess a revision which is the same behavior as before. On the contrary we now have a way to flag resolved revisions outside the downloader. I already started using this in a separate PR for projects, because if the analyzer runs on a VCS working tree we always know that the revisions of detected projects are resolved.

The tests already worked for the main commit which moves the property.

Turns out I still have to fix some tests. I wrongly thought that we always execute all test tasks, but it seems that either Gradle stops executing tests tasks if one of them had failing tests, or that we have issues with test reporting on Azure, so everytime I fixed tests a new set of failing tests from another module popped up.

sschuberth · 2021-06-10T09:09:59Z

analyzer/src/funTest/kotlin/integration/DrupalIntegrationFunTest.kt

+            type = VcsType.GIT,
+            url = "https://github.com/drupal/drupal.git",
+            revision = "4a765491d80d1bcb11e542ffafccf10aef05b853",
+            isResolvedRevision = true


As discussed in the developer meeting, isVerifiedRevision or isConfirmedRevision would probably fit the semantics better.

I have renamed it to isVerifiedRevision and also removed the last commit because you mentioned in the meeting today that NuGet takes the commit SHA1 from metadata so it would be wrong to set the flag there.

As discussed today I have now completely removed the isVerifiedRevision property so that we can address this topic later in a more sophisticated way.

analyzer/src/test/kotlin/managers/GoModTest.kt

model/src/main/kotlin/Provenance.kt

…esults ORT does not support source artifacts for projects, therefore we can safely assume that the provenance of a scan result for a project is always a `RepositoryProvenance`. Note that this assumption was already made further down in the same function. Signed-off-by: Martin Nonnenmacher <martin.nonnenmacher@here.com>

The function erroneously compared the `revision` and the `resolvedRevision` of the provided `vcsInfo`. This was wrong, because it was instead supposed to check if the `revision` of a nested repository matches the `resolvedRevision` of the provided `vcsInfo`. While on it, also change the signature of the function to take a `RepositoryProvenance` instead of a `VcsInfo` to make clear that it is expected to contain a `resolvedRevision`. This is also a preparation for an upcoming commit which will move the `resolvedRevision` property from `VcsInfo` to `RepositoryProvenance`. Signed-off-by: Martin Nonnenmacher <martin.nonnenmacher@here.com>

This is a preparation for a subsequent commit which will remove `resolvedRevision` from `VcsInfo` and move it to `RepositoryProvenance` instead. Signed-off-by: Martin Nonnenmacher <martin.nonnenmacher@here.com>

analyzer/src/funTest/kotlin/integration/DrupalIntegrationFunTest.kt

The `resolvedRevision` property was supposed to be only set by the downloader because it should contain the revision resolved from a local VCS working tree (c87b658). This was not ideal, because this requirement could easily be overlooked when creating `VcsInfo`s in the analyzer. It also made it complex to match the provenance of a scan result against the `VcsInfo` of `Project`s and `Package`s. Moving the property to `RepositoryProvenance` makes it clear which revision is provided by the analyzer and which revision is provided by the downloader which simplifies the matching. The intention to set `resolvedRevision` only by the downloader previously lead to problems, especially because there was also the demand that package managers can signal that a revision was already resolved (see 48d2cbb, 32681e1, 835b197). This commit removes the confusion about the property by moving it to the correct place in `RepositoryProvenance`. Signed-off-by: Martin Nonnenmacher <martin.nonnenmacher@here.com>

mnonnenmacher · 2021-06-15T14:27:53Z

I have marked this as a breaking change because it partly invalidates stored scan results. In one test with ~500 stored scan results ~400 could still be matched, the other ~100 had to be scanned again. After re-scan of those packages the results can be retrieved from the storage again.

sschuberth · 2021-06-15T14:48:56Z

I have marked this as a breaking change because it partly invalidates stored scan results.

I had a similar discussion with @MarcelBochtler in one of my PRs, and as this is not a change that requires our users to react, nor does it change the results. It would only be perceived as a (temporary) performance "regression". So from my point of view, we could drop the "breaking change" label.

model/src/main/kotlin/VcsInfo.kt

model/src/main/kotlin/Provenance.kt

mnonnenmacher · 2021-06-16T18:27:03Z

I have marked this as a breaking change because it partly invalidates stored scan results.

I had a similar discussion with @MarcelBochtler in one of my PRs, and as this is not a change that requires our users to react, nor does it change the results. It would only be perceived as a (temporary) performance "regression". So from my point of view, we could drop the "breaking change" label.

Makes sense, I have added the release notes label instead.

mnonnenmacher · 2021-06-17T07:15:36Z

I have marked this as a breaking change because it partly invalidates stored scan results.

I had a similar discussion with @MarcelBochtler in one of my PRs, and as this is not a change that requires our users to react, nor does it change the results. It would only be perceived as a (temporary) performance "regression". So from my point of view, we could drop the "breaking change" label.

Makes sense, I have added the release notes label instead.

I have re-added the breaking change label, because if ORT is used programmatically the API change does require adaption.

mnonnenmacher force-pushed the vcs-info-remove-resolved-revision branch 5 times, most recently from 3f93abf to cd95a2b Compare June 9, 2021 15:51

mnonnenmacher mentioned this pull request Jun 9, 2021

WorkingTree: Make clear that returned revisions are resolved #4142

Closed

mnonnenmacher marked this pull request as ready for review June 9, 2021 15:59

mnonnenmacher requested a review from a team as a code owner June 9, 2021 15:59

mnonnenmacher force-pushed the vcs-info-remove-resolved-revision branch from cd95a2b to 77ef9e0 Compare June 9, 2021 19:51

sschuberth reviewed Jun 9, 2021

View reviewed changes

sschuberth reviewed Jun 10, 2021

View reviewed changes

mnonnenmacher force-pushed the vcs-info-remove-resolved-revision branch 6 times, most recently from 6668f86 to cda30e3 Compare June 11, 2021 16:03

sschuberth requested changes Jun 11, 2021

View reviewed changes

analyzer/src/test/kotlin/managers/GoModTest.kt Outdated Show resolved Hide resolved

model/src/main/kotlin/Provenance.kt Outdated Show resolved Hide resolved

model/src/main/kotlin/Provenance.kt Outdated Show resolved Hide resolved

mnonnenmacher force-pushed the vcs-info-remove-resolved-revision branch from cda30e3 to 3737398 Compare June 15, 2021 08:40

mnonnenmacher added 3 commits June 15, 2021 10:40

VcsMatcher: Match against RepositoryProvenance instead of VcsInfo

e01c784

This is a preparation for a subsequent commit which will remove `resolvedRevision` from `VcsInfo` and move it to `RepositoryProvenance` instead. Signed-off-by: Martin Nonnenmacher <martin.nonnenmacher@here.com>

mnonnenmacher force-pushed the vcs-info-remove-resolved-revision branch from 3737398 to 7a09d26 Compare June 15, 2021 08:41

mnonnenmacher requested a review from sschuberth June 15, 2021 08:53

sschuberth requested changes Jun 15, 2021

View reviewed changes

analyzer/src/funTest/kotlin/integration/DrupalIntegrationFunTest.kt Show resolved Hide resolved

mnonnenmacher force-pushed the vcs-info-remove-resolved-revision branch from 7a09d26 to bd86932 Compare June 15, 2021 14:25

mnonnenmacher added the breaking change label Jun 15, 2021

mnonnenmacher requested a review from sschuberth June 15, 2021 14:28

sschuberth approved these changes Jun 15, 2021

View reviewed changes

fviernau requested changes Jun 16, 2021

View reviewed changes

model/src/main/kotlin/VcsInfo.kt Show resolved Hide resolved

model/src/main/kotlin/Provenance.kt Show resolved Hide resolved

mnonnenmacher added release notes and removed breaking change labels Jun 16, 2021

fviernau approved these changes Jun 16, 2021

View reviewed changes

mnonnenmacher added the breaking change label Jun 17, 2021

mnonnenmacher merged commit abd4157 into master Jun 17, 2021

mnonnenmacher deleted the vcs-info-remove-resolved-revision branch June 17, 2021 07:16

sschuberth mentioned this pull request Aug 26, 2021

Resolve symbolic VCS revisions already in the analyzer #4398

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move resolvedRevision from VcsInfo to Provenance #4132

Move resolvedRevision from VcsInfo to Provenance #4132

mnonnenmacher commented Jun 8, 2021 •

edited

Loading

mnonnenmacher commented Jun 9, 2021

sschuberth commented Jun 9, 2021

mnonnenmacher commented Jun 9, 2021

sschuberth left a comment

sschuberth Jun 9, 2021

sschuberth Jun 9, 2021

sschuberth Jun 9, 2021

mnonnenmacher Jun 10, 2021

sschuberth Jun 10, 2021

mnonnenmacher Jun 10, 2021

mnonnenmacher Jun 11, 2021

mnonnenmacher commented Jun 15, 2021

sschuberth commented Jun 15, 2021

mnonnenmacher commented Jun 16, 2021

mnonnenmacher commented Jun 17, 2021

Move resolvedRevision from VcsInfo to Provenance #4132

Move resolvedRevision from VcsInfo to Provenance #4132

Conversation

mnonnenmacher commented Jun 8, 2021 • edited Loading

mnonnenmacher commented Jun 9, 2021

sschuberth commented Jun 9, 2021

mnonnenmacher commented Jun 9, 2021

sschuberth left a comment

Choose a reason for hiding this comment

sschuberth Jun 9, 2021

Choose a reason for hiding this comment

sschuberth Jun 9, 2021

Choose a reason for hiding this comment

sschuberth Jun 9, 2021

Choose a reason for hiding this comment

mnonnenmacher Jun 10, 2021

Choose a reason for hiding this comment

sschuberth Jun 10, 2021

Choose a reason for hiding this comment

mnonnenmacher Jun 10, 2021

Choose a reason for hiding this comment

mnonnenmacher Jun 11, 2021

Choose a reason for hiding this comment

mnonnenmacher commented Jun 15, 2021

sschuberth commented Jun 15, 2021

mnonnenmacher commented Jun 16, 2021

mnonnenmacher commented Jun 17, 2021

mnonnenmacher commented Jun 8, 2021 •

edited

Loading