-
Notifications
You must be signed in to change notification settings - Fork 175
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
Add possibility to set downstream projects for child projects. #106
Add possibility to set downstream projects for child projects. #106
Conversation
ParameterizedDependency.add(owner, project, config, graph); | ||
if (config.triggerFromChildProjects() && owner instanceof ItemGroup) { | ||
ItemGroup parent = (ItemGroup) owner; | ||
for (AbstractProject child : (Collection<AbstractProject>)parent.getItems()) { |
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.
This cast is strictly speaking unsafe as there is no guarantee it contains AbstractProject
s.
Yes, sorry my IDE likes to add comments. |
762a4ff
to
c5f5c40
Compare
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.
It would be great to have a more detailed explanation of the use-case for such change. I'm not sure which new features it introduces since such logic was implementable in Publisher before IIRC
this.triggerFromChildProjects = triggerFromChildProjects; | ||
} | ||
|
||
public BuildTriggerConfig(String projects, ResultCondition condition, |
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.
Maybe this method needs to be deprecated
if (config.triggerFromChildProjects() && owner instanceof ItemGroup) { | ||
ItemGroup<Item> parent = (ItemGroup) owner; | ||
for (Item item : parent.getItems()) { | ||
if(item instanceof AbstractProject){ |
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 would vote against using AbstractProject in the new code, but it requires rework of ParameterizedDependency
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.
Thank you for all response, I put it into my code. I am afraid that only reworking ParametrizedDependency does not help.... https://github.com/jenkinsci/jenkins/blob/master/core/src/main/java/hudson/model/DependencyGraph.java#L394
@@ -116,6 +128,10 @@ public ResultCondition getCondition() { | |||
public boolean getTriggerWithNoParameters() { | |||
return triggerWithNoParameters; | |||
} | |||
|
|||
public boolean triggerFromChildProjects(){ |
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.
Replace by isTriggerFromChildProjects
to follow the common naming convention
@@ -630,6 +646,10 @@ public String getDisplayName() { | |||
return Hudson.getInstance().<AbstractBuildParameterFactory, | |||
Descriptor<AbstractBuildParameterFactory>>getDescriptorList(AbstractBuildParameterFactory.class); | |||
} | |||
|
|||
public boolean isItemGroup(AbstractProject project){ |
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.
Better to Restrict this method by @Restricted(DoNotUse.class)
since it is only for UI
Oleg, this functionality should able to trigger the same job for every configuration for processing the results of particular configurations and uploading them to another server. It is true that we can use publishers but in case of matrix job, the publisher is used only for whole matrix - not for particular configuration. But we need to load results of every particular configuration separated. |
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.
This change looks ok to me. It sounds like a more granular approach was needed for certain jobs, and this PR fixes those issues.
No description provided.