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

[JENKINS-26050] Fix Workflow compatibility for Parameterized Trigger #87

Merged
merged 16 commits into from Aug 10, 2015

Conversation

@svanoort
Copy link
Member

commented Jul 20, 2015

Per JENKINS-26050 this fixes workflow compatibility, and adds unit tests for same.

@reviewbybees

import hudson.model.AbstractBuild;
import hudson.model.AbstractDescribableImpl;
import hudson.model.TaskListener;
import hudson.model.*;

This comment has been minimized.

Copy link
@jtnord

jtnord Jul 20, 2015

Member

prefer explicit changes over * imports.
This file has no (meaningful) changes.

config.perform(build, launcher, listener);
alreadyFired.add(config);
}
}

This comment has been minimized.

Copy link
@jtnord

jtnord Jul 20, 2015

Member

the indentation here is completely out of sync with the rest of the method.

List<Job> jobs = config.getProjectList(build.getRootBuild().getProject().getParent(), build.getEnvironment(listener));
for (Job j : jobs) {
if (!(j instanceof AbstractProject)) {
hasNonAbstractProject = true;

This comment has been minimized.

Copy link
@jtnord

jtnord Jul 20, 2015

Member

seems like you should break here.

for (BuildTriggerConfig config : configs) {
config.perform(build, launcher, listener);
if (!alreadyFired.contains(config)) {

This comment has been minimized.

Copy link
@jtnord

jtnord Jul 20, 2015

Member

indentation...

This comment has been minimized.

Copy link
@jtnord

jtnord Jul 20, 2015

Member

maybe I missed something - but there is no point in this check - or the loop above as you will always call config.perform with the same arguments - either here (if its not alreadyFired, or above.)

This comment has been minimized.

Copy link
@svanoort

svanoort Jul 20, 2015

Author Member

@jtnord It's subtle -- the canDeclare method is sufficient for most cases, where all builds will be handled via the dependency graph and all projects extend AbstractProject.

We're only explicitly calling the trigger perform method if the trigger includes workflow jobs (which DependencyGraph does not support) OR if canDeclare is false... in cases where both are true, I'm trying to avoid performing the same build twice.

This comment has been minimized.

Copy link
@jtnord

jtnord Jul 20, 2015

Member

so canDeclare will always be false for workflow (as it doesn't support the dependencyGraph), so the else will always be entered. So I'm still confused.

This comment has been minimized.

Copy link
@svanoort

svanoort Jul 20, 2015

Author Member

@jtnord Fair enough, it's rather confusing!

The upstream project is always an AbstractProject (you can't add the trigger on a workflow job, but workflow has its own mechanism to fire downstream builds). canDeclare is invoked on the upstream project, an AbstractProject, simply to see if the upstream can participate in the DependencyGraph, which might be true or false.

Regardless of this, we need to ensure that workflow jobs are triggered. I'm not going to guarantee there aren't edge cases until the tests are reviewed, but this part at least should work.

I hope that makes the rationale a bit clearer? I'd be happy to do a hangout if it still seems iffy to you.

@@ -307,9 +293,8 @@ private static void resolveProject(AbstractBuild build, SubProjectData subProjec
List<Action> actions = getBaseActions(
ImmutableList.<AbstractBuildParameters>builder().addAll(configs).addAll(addConfigs).build(),
build, listener);
for (AbstractProject project : getProjectList(build.getRootBuild().getProject().getParent(),env)) {
for (Job project : getProjectList(build.getRootBuild().getProject().getParent(),env)) {

This comment has been minimized.

Copy link
@jtnord

jtnord Jul 20, 2015

Member

indentation

@jtnord

This comment has been minimized.

Copy link
Member

commented Jul 20, 2015

🐛s for the import *, and not matching the current indentation scheme.

Theres also an open question on the BuildTrigger.java

for (BuildTriggerConfig config : configs)
for (AbstractProject project : config.getProjectList(owner.getParent(),null))
for (BuildTriggerConfig config : configs) {
List<AbstractProject> projectList = Util.filter(config.getProjectList(owner.getParent(), null), AbstractProject.class);

This comment has been minimized.

Copy link
@amuniz

amuniz Jul 20, 2015

Member

Are you expecting to get Workflow jobs here? If so, I think it will not work.

This comment has been minimized.

Copy link
@svanoort

svanoort Jul 20, 2015

Author Member

@amuniz Thanks for the feedback. Workflow jobs are explicitly fired via the BuildTrigger.perform method; they don't participate in the DependencyGraph mechanism (core APIs don't support WorkflowJobs -- it's been raised as a potential enhancement, but not yet).

* @return can be empty but never null.
*/
public abstract List<AbstractBuildParameters> getParameters(AbstractBuild<?,?> build, TaskListener listener)
public abstract List<AbstractBuildParameters> getParameters(AbstractBuild<?, ?> build, TaskListener listener)

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev Jul 20, 2015

Member

Please avoid such formatting changes if you don't change the line. They make the review and merges more difficult

import hudson.model.AbstractProject;
import hudson.model.Action;
import hudson.model.BuildListener;
import hudson.model.*;

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev Jul 20, 2015

Member

No, the common practice in Jenkins is to refer all imports to prevent conflicts of new classes

@@ -53,14 +48,14 @@ public BlockingBehaviour getBlock() {
}

@Override
public ListMultimap<AbstractProject, Future<AbstractBuild>> perform2(AbstractBuild<?, ?> build, Launcher launcher, BuildListener listener) throws InterruptedException, IOException {
ListMultimap<AbstractProject, Future<AbstractBuild>> futures = super.perform2(build, launcher, listener);
public ListMultimap<Job, Future<Run>> perform2(AbstractBuild<?, ?> build, Launcher launcher, BuildListener listener) throws InterruptedException, IOException {

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev Jul 20, 2015

Member

Binary backward compatibility breach. Collections cannot be promoted in such way

@@ -111,7 +97,7 @@ public boolean getTriggerWithNoParameters() {
* @deprecated
* Use {@link #getProjectList(ItemGroup, EnvVars)}
*/
public List<AbstractProject> getProjectList(EnvVars env) {
public List<Job> getProjectList(EnvVars env) {

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev Jul 20, 2015

Member

Backward compatibility

public List<AbstractProject> getProjectList(ItemGroup context, EnvVars env) {
List<AbstractProject> projectList = new ArrayList<AbstractProject>();
projectList.addAll(Items.fromNameList(context, getProjects(env), AbstractProject.class));
public List<Job> getProjectList(ItemGroup context, EnvVars env) {

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev Jul 20, 2015

Member

Backward compatibility

@@ -322,19 +307,18 @@ private static void resolveProject(AbstractBuild build, SubProjectData subProjec
return Collections.emptyList();
}

public ListMultimap<AbstractProject, Future<AbstractBuild>> perform2(AbstractBuild<?, ?> build, Launcher launcher, BuildListener listener) throws InterruptedException, IOException {
public ListMultimap<Job, Future<Run>> perform2(AbstractBuild<?, ?> build, Launcher launcher, BuildListener listener) throws InterruptedException, IOException {

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev Jul 20, 2015

Member

Backward compatibility

@@ -403,15 +387,35 @@ protected Cause createUpstreamCause(AbstractBuild<?, ?> build) {
return new UpstreamCause((Run) build);

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev Jul 20, 2015

Member

You should remove obsolete class casts

import hudson.model.ParameterDefinition;
import hudson.model.ParameterValue;
import hudson.model.ParametersDefinitionProperty;
import hudson.model.*;

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev Jul 20, 2015

Member

Nope, it should not be done in such way

This comment has been minimized.

Copy link
@jglick

jglick Jul 22, 2015

Member

There is no general styleguide for Jenkins plugins. My personal preference is to avoid star imports (except for import static), and keep all imports simply sorted in one block—after all, imports are for javac and your IDE to read, not for humans.

This comment has been minimized.

Copy link
@jtnord

jtnord Aug 5, 2015

Member

if I was a maintainer of this I would reject such a change.
You may get lucky...

@oleg-nenashev

This comment has been minimized.

Copy link
Member

commented Jul 20, 2015

🐛 for binary compatibility changes. If you want to go in such way, you'll need much more justification of the change.

@jenkinsadmin

This comment has been minimized.

Copy link
Member

commented Jul 20, 2015

Thank you for a pull request! Please check this document for how the Jenkins project handles pull requests

@svanoort

This comment has been minimized.

Copy link
Member Author

commented Jul 20, 2015

@oleg-nenashev Can you please explain a bit more about where we expect to hit issues from changing method signatures and breaking binary compatibility?

Edit: never mind, I think I see the issue, the plugin library is imported into a couple other plugins, which I was not expecting.

@jglick jglick changed the title [WIP] JENKINS-26050 fix workflow compatibility for parameterized trigger [JENKINS-26050] Fix Workflow compatibility for Parameterized Trigger Jul 22, 2015
return project;
}
};
return parameterizedJobMixIn.scheduleBuild2(quietPeriod, queueActions.toArray(new Action[queueActions.size()]));

This comment has been minimized.

Copy link
@jglick

jglick Jul 22, 2015

Member

FYI jenkinsci/jenkins#1771 would be nicer here. I would recommend leaving in a TODO comment to that effect.

@reviewbybees

This comment has been minimized.

Copy link

commented Jul 27, 2015

This pull request originates from a CloudBees employee. At CloudBees, we require that all pull requests be reviewed by other CloudBees employees before we seek to have the change accepted. If you want to learn more about our process please see this explanation.

@oleg-nenashev

This comment has been minimized.

Copy link
Member

commented Jul 29, 2015

removed the bug after the discussion

@jglick

This comment has been minimized.

Copy link
Member

commented Jul 29, 2015

BuildTriggerConfigTest uncompilable.

@jglick

This comment has been minimized.

Copy link
Member

commented Jul 29, 2015

🐛 for failure to include src/test/java/ in rename refactoring 😮

@stephenc

This comment has been minimized.

Copy link
Member

commented Jul 29, 2015

🐛 build failing

@stephenc

This comment has been minimized.

Copy link
Member

commented Jul 29, 2015

🐝 (conditional on Jenkins saying it builds)

@jglick

This comment has been minimized.

Copy link
Member

commented Jul 29, 2015

🐝 (also conditional on tests passing of course)

import hudson.model.SimpleParameterDefinition;
import hudson.model.StringParameterDefinition;
import hudson.model.StringParameterValue;
import hudson.model.*;

This comment has been minimized.

Copy link
@jtnord

jtnord Aug 5, 2015

Member

if I was the maintainer of this plugin I would not be happy with this change.
YMMV.

public void testGetProjectListWithWorkflow() throws Exception {
Project<?, ?> masterProject = createFreeStyleProject("project");
WorkflowJob p = jenkins.createProject(WorkflowJob.class, "workflowproject");
p.setDefinition(new CpsFlowDefinition("println('hello')"));

This comment has been minimized.

Copy link
@jtnord

jtnord Aug 5, 2015

Member

nit: should this not be echo - so it uses workflow rather than just some groovy?

This comment has been minimized.

Copy link
@jglick

jglick Aug 5, 2015

Member

Actually Workflow hacks things a bit so that println is just an alias for echo. It is preferable to use echo directly.

@jtnord

This comment has been minimized.

Copy link
Member

commented Aug 5, 2015

🐝 on the code. If I was the maintainer I would not be happy with the change to import * - however I am not and you may get lucky 😄

svanoort added 2 commits Aug 4, 2015
…elly that used old idioms

Repairs defect found in demo that for workflow jobs, UI shows "project is not buildable" error in UI
Also, remove some trailing whitespace
@svanoort

This comment has been minimized.

Copy link
Member Author

commented Aug 5, 2015

@jtnord I fixed the imports -- looks like one slipped through from earlier.
I've since told my IDE to stop trying to be "helpful" by collapsing imports unless it has 99 from the same pkg.

@jglick This includes the fix for the UI checker irritation

@jglick

This comment has been minimized.

🐜 if !(project instanceof ParameterizedJobMixIn.ParameterizedJob) then you will not be able to trigger that job so this should be treated as an error (BuildTrigger.NotBuildable as above).

@jglick

This comment has been minimized.

I guess this comment was copied from hudson.tasks.BuildTrigger where it made sense.

@jglick

This comment has been minimized.

Copy link
Member

commented Aug 5, 2015

🐝

Move up parameterized job instanceof check for unbuildable
Remove irrelevant comment on ACL check
@jglick

This comment has been minimized.

Copy link
Member

commented Aug 5, 2015

🐝

}
if(!(item instanceof AbstractProject)){
if(!(item instanceof Job) || !(item instanceof ParameterizedJobMixIn.ParameterizedJob)) {

This comment has been minimized.

Copy link
@amuniz

amuniz Aug 5, 2015

Member

What's intended to be handled here? Would it be possible that the item were not a Job/AbstractProject?

This comment has been minimized.

Copy link
@jglick

jglick Aug 5, 2015

Member

Sure, for example a Folder is an Item but not a Job, and an ExternalJob is a Job but not a ParameterizedJob.

This comment has been minimized.

Copy link
@amuniz

amuniz Aug 5, 2015

Member

OK. Thanks.

@amuniz

This comment has been minimized.

Copy link
Member

commented Aug 5, 2015

I think this is 🐝 (beyond the little question I put there)

@jglick

This comment has been minimized.

Copy link
Member

commented Aug 5, 2015

@reviewbybees done

(since @oleg-nenashev forgot to add an amending bee before going on vacation)

@svanoort

This comment has been minimized.

Copy link
Member Author

commented Aug 10, 2015

After checking with active community members to see if they have concerns, I'm going ahead and merging this since we have users waiting on it.

svanoort added a commit that referenced this pull request Aug 10, 2015
[JENKINS-26050] Fix Workflow compatibility for Parameterized Trigger
@svanoort svanoort merged commit 5f04921 into jenkinsci:master Aug 10, 2015
1 check passed
1 check passed
Jenkins This pull request looks good
Details
@oleg-nenashev

This comment has been minimized.

Copy link
Member

commented Aug 10, 2015

@svanoort
According to the Jenkins process, you should try to contact the plugin's owner and wait for 1 week for the response before going forward. IIRC @ikedam was the de-facto owner of the plugin for a while.

Anyway, waiting for a response in the mailing list on the weekend was not enough IMHO.

@svanoort

This comment has been minimized.

Copy link
Member Author

commented Aug 10, 2015

@oleg-nenashev To capture not visible discussion, I'd reached out to KK (who is still formally listed as the maintainer, even if community members) and olivergondza (who's done a lot of the releases and commits recently, more than ikedam) with positive responses from both. In this case we also have at least one larger Jenkins user waiting on this to become available, so I'm trying to juggle competing concerns here.

I'll certainly bear in mind that community process is to wait a week (in absence of actively solicited feedback as I received in this case), and will try to make that feedback more visible in general.

@oleg-nenashev

This comment has been minimized.

Copy link
Member

commented Aug 13, 2015

Thanks for the clarification and for all your efforts to get it merged!
👍

tonyfinn added a commit to tonyfinn/parameterized-trigger-plugin that referenced this pull request Aug 21, 2015
… items from the iterator were still being cast to AbstractProject.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
8 participants
You can’t perform that action at this time.