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

ConditionalBuilder now implements DependencyDeclarer #12

Merged
merged 3 commits into from
Jan 12, 2017

Conversation

TWestling
Copy link
Member

SingleConditionalBuilder already implements the correct
DependencyDeclarer. This change makes it so that
ConditionalBuilder does the same.

This is needed to get downstream functionality of e.g. the Junit plugin aggregation and downstream- buildview to work.

SingleConditionalBuilder already implements the correct
DependencyDeclarer. This change makes it so that
ConditionalBuilder does the same.
@TWestling
Copy link
Member Author

@imod Could you take a look at this? It is basically the same as #10 but for the multi step instead of single step.

@imod
Copy link
Member

imod commented Oct 24, 2016

@TWestling looks good to me - I'll merge is in the next couple of days

All build steps get added to the dependency graph,
no matter what the conditions are. This change remedies this
by using the ConditionalDependencyGraphWrapper from the
flexible-publish plugin.
@TWestling
Copy link
Member Author

@imod I noticed that this doesn't work as is for e.g. the parameterized trigger, nor does it work for pull #10 Since we add every builder to the dependencygraph, even if the condition returns false, the graph will make sure that it runs anyway. This is solved in the flexible-publish plugin so the simplest solution is to add a dependency to flexible-publish. I'll add that change to this pull request, please have a look.

@imod
Copy link
Member

imod commented Oct 27, 2016

@TWestling sorry, I don't like the dependency to the flexible-publish - can't we just do it the same way as it is done there?

@TWestling
Copy link
Member Author

@imod I was thinking you would say that :)
It looks like the support is mostly done by 2 classes so sure, I can copy those over here. I will update this PR with the changes, probably next week.

@imod
Copy link
Member

imod commented Nov 17, 2016

@TWestling any plans to finish this?

@TWestling
Copy link
Member Author

@imod Yes, this slipped my mind as we started using my version of the plugin and then I forgot about it. I'll reopen it now.

Copied the needed classes from the flexible publish plugin
and removed the dependency from this plugin.
@TWestling
Copy link
Member Author

@ikedam I've copied two of your classes from the flexible-publish-plugin in order to get the dependency graph to work as it should. I copied them in as they were and just added a note regarding where they came from, I hope this is ok.

@ikedam
Copy link
Member

ikedam commented Jan 10, 2017

@TWestling Looks good to me.

@TWestling
Copy link
Member Author

@imod I removed the dependency to the flexible-publisher plugin, could you have a look?

@imod
Copy link
Member

imod commented Jan 12, 2017

👍

@imod imod merged commit 354bcce into jenkinsci:master Jan 12, 2017
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.

3 participants