-
Notifications
You must be signed in to change notification settings - Fork 103
feat: task configuration #491
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
…Purge and Update tasks
|
Thanks for the effort/thought that went into this. Was this discussed somewhere as an approach? I am just a contributor and long time user, but I'm not in support of this approach. I understand the motivation, as there are a number of problems with the current approach including
Exposing The concerns I have here are a few aspects (not going into detail, just broad inferences for now)
If people really want to do this; it is still mostly possible to "break glass" and set raw To me, that is perfectly good enough, and we should not make it easier to cut through all the layers to ODC internals. So it is unfortunately a -1 for me. If we wanted to consider a middle-ground that I might find slightly more tolerable, we might consider an extra option for power-user-only ability to set a properties file to bootstrap settings via at the below, however given none of the other tools/plugins seem to utilise this option, perhaps that's not intended either. dependency-check-gradle/src/main/groovy/org/owasp/dependencycheck/gradle/tasks/ConfiguredTask.groovy Lines 51 to 52 in 70582dc
|
|
@chadlwilson thank you for such a detailed answer! I really appreciate your time and effort spent on this! Let me justify myself since my change probably brings too many things at once.
I'm not sure about Maven plugin but this is a general Gradle approach - using extension to configure task inputs:
With this approach tasks can be registered and configured separately. Also here is a user request: #484. As I already mentioned inputs are used by Gradle e.g. for input validation, up-to-date checks and task dependencies resolution: And here is the problem with At this point I assume I completely understand your point and absolutely agree with the leaky abstraction.
However At the end of the day extension DSL is still in place and should be the main approach for plugin configuration while task configuration is more like an advanced technique with some tradeoffs like possibility to break something or future version incompatibility. I'm happy this conversation is started and hope we can find common solution. |
|
My concern above isn't making any particular comment about your task configuration approach (although why do things differently to #487 without contributing to get that work merged is a bit odd). The concern is mainly the generic Settings binding and coupling. Software is built on abstractions and layers and not everything is intended to be exposed to everywhere else directly. ODC core is a relatively small project (maintainer wise) and does not seem to be religious about defining its Java API. Basically everything seems to be public by default, there are few interfaces,.the impls are not split, and so personally I don't think this can be assumed to mean anything. It's an obvious truth that the closer you move something to end users, the less likely you can change it without annoying people. So the question is - is it OK to make all Settings visible,. discoverable (via Kotlin mainly) and usable by end users in the current state? To me, Settings is mainly intended to be for plugin/API developers. Even ODCs own CLI doesn't expose every settings key by some kind of magic mapping to a CLI arg. I of course understand well that inputs are the Gradle way, but I still believe that directly allowing setting every single Setting via Inputs is a bad idea for the reasons I outlined above. If this is intended for advanced usages, I think it's fine to add friction via System properties. Personally I don't think a project should make it easier for users to do things that make maintainers lives harder, especially given how few people have even used the current friction as a reason to help and address some of the missing settings. So the debate here for others to decide is whether this does indeed make maintenance harder, or whether I am inventing problems Anyway, it's not my project, and I don't have merge permissions - I'm just an occasional (minor) contributor, issue triager and user who has an opinion :) |
jeremylong
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 have to agree with @chadlwilson - I'm not a fan of exposing the Settings object. If additional settings should be exposed, they should be added to an extension.
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.
Pull request overview
This PR introduces a task configuration system that exposes low-level Settings configuration for DependencyCheck tasks. The main purpose is to enable configuration of Settings keys not available through DependencyCheckExtension or to support new versions of dependency-check-core with older plugin versions.
Key changes:
- Introduces
DependencyCheckTaskConfigandAnalyzeTaskConfiginput hierarchy for configuring tasks - Refactors
ConfiguredTaskto build settings from a map rather than directly calling settings methods - Adds
config()methods to all tasks allowing per-task configuration overrides
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/main/groovy/org/owasp/dependencycheck/gradle/extension/DependencyCheckTaskConfig.groovy | New interface defining base task configuration with failOnError property and settings map |
| src/main/groovy/org/owasp/dependencycheck/gradle/extension/AnalyzeTaskConfig.groovy | New abstract class extending DependencyCheckTaskConfig with analyze-specific properties |
| src/main/groovy/org/owasp/dependencycheck/gradle/extension/AdditionalCpe.groovy | Marks getName() as @internal to prevent it from affecting task up-to-date checks |
| src/main/groovy/org/owasp/dependencycheck/gradle/tasks/ConfiguredTask.groovy | Major refactoring to use config object pattern and build settings map; simplified proxy configuration |
| src/main/groovy/org/owasp/dependencycheck/gradle/tasks/AbstractAnalyze.groovy | Updated to use AnalyzeTaskConfig and expose config() method |
| src/main/groovy/org/owasp/dependencycheck/gradle/tasks/Update.groovy | Added config() method for task-level configuration |
| src/main/groovy/org/owasp/dependencycheck/gradle/tasks/Purge.groovy | Added config() method for task-level configuration |
| src/test/groovy/org/owasp/dependencycheck/gradle/DependencyCheckPluginIntegSpec.groovy | Added integration test verifying multiple analyze tasks can be configured independently |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/test/groovy/org/owasp/dependencycheck/gradle/DependencyCheckPluginIntegSpec.groovy
Outdated
Show resolved
Hide resolved
src/main/groovy/org/owasp/dependencycheck/gradle/extension/AnalyzeTaskConfig.groovy
Outdated
Show resolved
Hide resolved
src/main/groovy/org/owasp/dependencycheck/gradle/extension/DependencyCheckTaskConfig.groovy
Outdated
Show resolved
Hide resolved
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
This PR exposes low-level
Settingsconfiguration for tasks.The main purpose is to have ability to configure
Settingskeys that are not available throughDependencyCheckExtensione.g. see #489 or to set up new version of dependency-check-core with old plugin version where extension DSL is not yet upgraded for new features.DependencyCheckTaskConfig/AnalyzeTaskConfiginput hierarchy was introduced to configure tasks and this should resolve #484 issue.DependencyCheckExtensionprovides conventions for task inputs that can be refined for specific task:Most of the changes are in
ConfiguredTask.Settingsvalues collected fromDependencyCheckExtensiontoMapPropertyofDependencyCheckTaskConfig.Groovy truth is used to replicate
setStringIfNotEmptycases coercing empty strings tonull:then nulls removed:
The only thing I worry about is
AnalyzeTaskConfig.isScanSetConfiguredflag.I simplified it to
!scanSet.emptyso it may break expectations regarding settingscanSetto empty list.Lastly I think it's worth mentioning that Analyze task inputs involved in gradle up-to-date checks which may make sense in some cases, however dependencyCheck result generally depends on external oss/nvd state so it is good to mention disable up-to-date checks option.
cc @ThomGeG