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

Don't declare sample plugin transitive dependencies #2470

Conversation

MarkEWaite
Copy link
Contributor

@MarkEWaite MarkEWaite commented Sep 6, 2023

Don't declare sample plugin transitive dependencies

Removes the following transitive dependency declarations from the sample plugin:

  • cloudbees-folder
  • favorite
  • pipeline-groovy-lib
  • pipeline-model-definition
  • variant
  • workflow-basic-steps
  • workflow-durable-task-step
  • workflow-multibranch

Each of those plugins is already a dependency of another plugin declared in the pom file of the sample plugin. No need to declare the plugin if it is a dependency of an already declared plugin.

Attempting to follow the guidance from the README that says:

Avoid adding transitive dependencies to sample-plugin/pom.xml. It is supposed to look as much as possible like a real plugin, and a real plugin should only declare its direct dependencies and not its transitive dependencies.

Testing done

mvn clean verify passes as well as the variants using profiles like mvn -P 2.387.x verify

Submitter checklist

  • Make sure you are opening from a topic/feature/bugfix branch (right side) and not your main branch!
  • Ensure that the pull request title represents the desired changelog entry
  • Please describe what you did
  • Link to relevant issues in GitHub or Jira
  • Link to relevant pull requests, esp. upstream and downstream changes
  • Ensure you have provided tests - that demonstrates feature works or fixes the issue

Removes declarations of:

* cloudbees-folder
* favorite
* pipeline-groovy-lib
* pipeline-model-definition
* variant
* workflow-basic-steps
* workflow-durable-task-step
* workflow-multibranch

Attempting to follow the guidance from the README that says:

> Avoid adding transitive dependencies to sample-plugin/pom.xml. It
> is supposed to look as much as possible like a real plugin, and a real
> plugin should only declare its direct dependencies and not its transitive
> dependencies.
@MarkEWaite MarkEWaite requested a review from a team as a code owner September 6, 2023 01:29
@MarkEWaite MarkEWaite added chore Reduces future maintenance full-test Test all LTS lines in this PR and do not halt upon first error. labels Sep 6, 2023
Copy link
Member

@basil basil left a comment

Choose a reason for hiding this comment

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

No need for full-test on this I think, since any changes to sample-plugin/ are already tested without full-test.

@MarkEWaite
Copy link
Contributor Author

No need for full-test on this I think, since any changes to sample-plugin/ are already tested without full-test.

Thanks. I'll cancel the CI job, remove the full-test label, and run the CI job again.

@MarkEWaite MarkEWaite removed the full-test Test all LTS lines in this PR and do not halt upon first error. label Sep 6, 2023
@MarkEWaite MarkEWaite enabled auto-merge (squash) September 6, 2023 02:03
@MarkEWaite MarkEWaite merged commit acad2e3 into jenkinsci:master Sep 6, 2023
6 checks passed
@jglick
Copy link
Member

jglick commented Sep 6, 2023

Thanks. There is no rigid guidance for plugin POMs regarding transitive vs. direct dependencies. mvn dependency:analyze will have lots of false positives & negatives due to the complexities of plugin class loading, and the fact that tests often rely on the presence of some plugin (for example to define a Pipeline step) without ever referring to its types. As a stylistic matter I would normally keep a dependency like pipeline-model-definition explicit even if some other test dep happened to pull it in, if my tests in fact involved Declarative Pipeline; or variant only if I were actually using @OptionalExtension.

@MarkEWaite MarkEWaite deleted the remove-sample-plugin-transitive-dependencies branch September 7, 2023 15:17
MarkEWaite added a commit to MarkEWaite/bom that referenced this pull request Sep 7, 2023
Plugin has a declarative Pipeline test in DeclarativePipelineTest.java

jenkinsci#2470 (comment)
provides more reasoning why the dependency should be included.

This reverts part of commit acad2e3.
MarkEWaite added a commit that referenced this pull request Sep 7, 2023
Plugin has a declarative Pipeline test in DeclarativePipelineTest.java

#2470 (comment)
provides more reasoning why the dependency should be included.

This reverts part of commit acad2e3.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
chore Reduces future maintenance
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants