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

Start using bom #121

Merged
merged 3 commits into from
Feb 27, 2020
Merged

Start using bom #121

merged 3 commits into from
Feb 27, 2020

Conversation

joseblas
Copy link
Contributor

@joseblas joseblas commented Feb 26, 2020

start using bom dependencies

@joseblas joseblas requested a review from jglick February 26, 2020 18:27
pom.xml Outdated Show resolved Hide resolved
pom.xml Outdated Show resolved Hide resolved
pom.xml Outdated Show resolved Hide resolved
@joseblas
Copy link
Contributor Author

@jglick should I try to flatten the dependencies? remove all the unnecessary ones?

@jglick
Copy link
Member

jglick commented Feb 26, 2020

remove all the unnecessary ones?

This. Sometimes a dependency was in a POM merely to satisfy RequireUpperBoundDeps; it was not being used as such. In these cases it should always have been in dependencyManagement, and can now be removed if the BOM declares it.

joseblas and others added 2 commits February 27, 2020 11:08
removing unnecessary reps

Co-Authored-By: Jesse Glick <jglick@cloudbees.com>
@joseblas joseblas merged commit f21a057 into jenkinsci:master Feb 27, 2020
@joseblas joseblas requested a review from jglick March 2, 2020 12:33
@joseblas
Copy link
Contributor Author

joseblas commented Mar 2, 2020

sorry @jglick, I got confused.

</dependency>
<dependency>
<groupId>org.jenkins-ci.plugins.workflow</groupId>
<artifactId>workflow-durable-task-step</artifactId>
Copy link
Member

Choose a reason for hiding this comment

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

This is definitely being used in tests and so ought to remain as an explicit dependency, even if it happens to also be pulled in from something else.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is a test dependency, I guess as far as tests are passing, is all good.

Copy link
Member

Choose a reason for hiding this comment

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

Looks like it is being pulled in via workflow-basic-steps. Just poor style generally to rely on transitive dependencies for things actually used directly. Anyway yes, if a future update of workflow-basic-steps drops that dep (as is quite possible in this case), updating to that would (I hope!) cause tests to start failing until you readded the explicit workflow-durable-task-step dep.

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'd say workflow-basic-steps is directly used and its dependencies. But, let me know if you want this added back.

Copy link
Member

Choose a reason for hiding this comment

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

The node step is used in a few tests. Not important, just FYI.

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.

2 participants