Skip to content
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(vcs): VCS configuration to support common and custom attributes #9271

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

wkl3nk
Copy link
Contributor

@wkl3nk wkl3nk commented Oct 10, 2024

While VCS implementations are already plugins, they are not yet configurable. VCS implementations require common configurations (e.g., revision, recursive) and should support also VCS-specific configurations if they are consumed via their API.

This allows to add functionality to individual VCS implementations without the need to implement them for all of them.

This refactoring keeps the common configuration attributes as they are, while VCS-specific configurations are stored generically in an options attribute.

The change also adds VCS-specific configuration options for Git: The strategy for checking out repositories with submodules now allows to only check out the first layer of submodules.

For more details, please have a look at the respective commits.

Fixes #8556.

Copy link

codecov bot commented Oct 10, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 67.49%. Comparing base (e612c50) to head (d9ef04f).
Report is 2 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff            @@
##               main    #9271   +/-   ##
=========================================
  Coverage     67.49%   67.49%           
  Complexity     1198     1198           
=========================================
  Files           241      241           
  Lines          8495     8495           
  Branches        900      900           
=========================================
  Hits           5734     5734           
  Misses         2399     2399           
  Partials        362      362           
Flag Coverage Δ
funTest-docker 60.59% <ø> (ø)
funTest-non-docker 33.63% <ø> (ø)
test 37.49% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@wkl3nk wkl3nk force-pushed the wkl3nk/make-vcs-plugins-configurable branch 5 times, most recently from 60b8d10 to ae31656 Compare October 11, 2024 14:06
@wkl3nk wkl3nk force-pushed the wkl3nk/make-vcs-plugins-configurable branch 5 times, most recently from 1cf9421 to ce5d30c Compare October 21, 2024 15:27
@wkl3nk wkl3nk marked this pull request as ready for review October 21, 2024 15:30
@wkl3nk wkl3nk requested a review from a team as a code owner October 21, 2024 15:30
@fviernau
Copy link
Member

I believe the options for configuring submodule recursion could be injected via package curations. Have you considered this?

@wkl3nk
Copy link
Contributor Author

wkl3nk commented Oct 22, 2024

@fviernau Hello, can you explain a little bit more please?

@fviernau
Copy link
Member

fviernau commented Oct 22, 2024

@fviernau Hello, can you explain a little bit more please?

I believe it makes sense to have the ability to configure this separate per dependency.
I expect the following trade-off / approach to be reasonable for some users:

  1. Enable submodule recursion by default
  2. Disable submodule recursion per package curation (after reviewing that this is ok, scan won't miss anything relevant)

@@ -387,7 +388,8 @@ abstract class VersionControlSystem(
workingTree: WorkingTree,
revision: String,
path: String = "",
recursive: Boolean = false
recursive: Boolean = false,
options: Options = emptyMap() // VCS-specific options only
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just adding an unused option to the function signature does not fix anything, so you should not say so in the commit message. Most importantly, these options are not read from the configuration file and passed on.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I hope I have improved the commit message now, also emphasising that this options are only available if the VCS plugin is consumed using the API.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

options are only available if the VCS plugin is consumed using the API.

I fear that's not enough. For the PR to be acceptable, VCS plugins should be configurable via config.yml entries, just like package managers like GradleInspector, independently of any programmatic use / ORT Server context.

plugins/version-control-systems/git/src/main/kotlin/Git.kt Outdated Show resolved Hide resolved
plugins/version-control-systems/git/src/main/kotlin/Git.kt Outdated Show resolved Hide resolved
plugins/version-control-systems/git/src/main/kotlin/Git.kt Outdated Show resolved Hide resolved
 While VCS implementations are already plugins, they are not yet
 configurable. VCS implementations require common configurations
 (e.g., `revision`, `recursive`) and should support also
 VCS-specific configurations if they are consumed via their API.
 This allows to add functionality to individual VCS implementations
 without the need to implement them for all of them.

 This refactoring keeps the common configurations attributes as
 they are, while VCS-specific configurations are stored
 generically in an `options` attribute.

Fixes oss-review-toolkit#8556.

Signed-off-by: Wolfgang Klenk <[email protected]>
 For large repositories with many layers of nested Git submodules, the
 download process can be very time-consuming and often results in
 duplicate projects in the tree of nested submodules.
 This feature introduces configuration options to limit the recursive
 checkout of nested Git submodules to the first layer, optimizing
 performance and reducing redundancy. Additionally, it also allows to
 limit the depth of commit history to fetch when downloading
 the projects.

Signed-off-by: Wolfgang Klenk <[email protected]>
@wkl3nk wkl3nk force-pushed the wkl3nk/make-vcs-plugins-configurable branch from ce5d30c to d9ef04f Compare October 22, 2024 11:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make VCS plugins configurable
3 participants