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-47780] Rename the symbol for TriggeredBuildSelector to "triggered" (was "upstream") #95

Conversation

ikedam
Copy link
Member

@ikedam ikedam commented Nov 3, 2017

https://issues.jenkins-ci.org/browse/JENKINS-47780

copyartifact-1.39 introduced symbols for pipelines (#88), but it caused a problem.

copyartifact-1.39 uses "upstream" for TriggeredBuildSelector, but "upstream" is already used for the symbol of ReverseBuildTrigger in Jenkins core:
https://github.com/jenkinsci/jenkins/blob/jenkins-2.73/core/src/main/java/jenkins/triggers/ReverseBuildTrigger.java#L192

workflow-cps usually decides the actual classes considering their contexts.
For example, when I declare like this,

copyArtifacts(projectName: "copiee", selector: upstream())

workflow-cps binds upstream to BuildSelector that is the type of the selector parameter.

But this looks not work for triggers.

pipeline {
    ...
    triggers {
        upstream (
            upstreamProjects: 'job1', threshold: hudson.model.Result.SUCCESS
        )
    }
   ...

Though apparently this upstream should be not BuildSelector but BuildTrigger, workflow-cps tries to resolve this upstream as ReverseBuildTrigger.

Installing copyartifact-1.39 cause ReverseBuildTrigger not work for declarative pipelines.

This request uses "triggered" instead of "upstream" for TriggeredBuildSelector.

@ikedam ikedam requested review from kohsuke and jglick November 3, 2017 18:56
private boolean allowUpstreamDependencies;

@DataBoundConstructor
public TriggeredBuildSelector() {
this(false);
}
Copy link
Member Author

Choose a reason for hiding this comment

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

Somehow the test written in #92 didn't failed even with copyArtifacts(projectName: 'copiee', selector: upstream()), it must have failed as DataBoundConstroctor with no parameters didn't exist.

Copy link
Member

@jglick jglick left a comment

Choose a reason for hiding this comment

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

While I would have a slight preference for the symbol triggered over upstream for this build selector (since it is more explicit), that is incompatible.

Anyway you should not have to do this. The Javadoc for @Symbol is explicit that a symbol need merely be unique within a given kind of extension. There should be no conflict between BuildSelectors and Triggers.

So I do not know what problem you are seeing in JENKINS-47780 but this is not the right solution; something else is wrong. Perhaps it is a bug in pipeline-model-definition; can you reproduce in Scripted Pipeline? CC @abayer

@abayer
Copy link
Member

abayer commented Nov 6, 2017

The Declarative issue is fixed in 1.2.3 regardless.

@ikedam
Copy link
Member Author

ikedam commented Nov 6, 2017

@abayer Unfortunately, pipeline-model-definition-plugin-1.2.3 doesn't look fix the problem yet.
Testing scripts described in JENKINS-47780 and JENKINS-47781 results following exception:

java.lang.ClassCastException: hudson.plugins.copyartifact.TriggeredBuildSelector cannot be cast to hudson.triggers.Trigger
	at org.jenkinsci.plugins.workflow.job.properties.PipelineTriggersJobProperty.<init>(PipelineTriggersJobProperty.java:67)
	at sun.reflect.NativeConstructorAccessorImpl.newInstance0(Native Method)
	at sun.reflect.NativeConstructorAccessorImpl.newInstance(NativeConstructorAccessorImpl.java:62)
	at sun.reflect.DelegatingConstructorAccessorImpl.newInstance(DelegatingConstructorAccessorImpl.java:45)
	at java.lang.reflect.Constructor.newInstance(Constructor.java:423)
	at org.codehaus.groovy.reflection.CachedConstructor.invoke(CachedConstructor.java:83)
	at org.codehaus.groovy.runtime.callsite.ConstructorSite$ConstructorSiteNoUnwrapNoCoerce.callConstructor(ConstructorSite.java:105)
	at org.codehaus.groovy.runtime.callsite.AbstractCallSite.callConstructor(AbstractCallSite.java:247)
	at org.jenkinsci.plugins.pipeline.modeldefinition.Utils.updateJobProperties(Utils.groovy:546)
	at org.jenkinsci.plugins.pipeline.modeldefinition.Utils$updateJobProperties$8.call(Unknown Source)
	at org.codehaus.groovy.runtime.callsite.CallSiteArray.defaultCall(CallSiteArray.java:48)
	at org.codehaus.groovy.runtime.callsite.AbstractCallSite.call(AbstractCallSite.java:113)
	at com.cloudbees.groovy.cps.sandbox.DefaultInvoker.methodCall(DefaultInvoker.java:19)
	at org.jenkinsci.plugins.pipeline.modeldefinition.ModelInterpreter.executeProperties(jar:file:/C:/Users/ikedam/.jenkins/plugins/pipeline-model-definition/WEB-INF/lib/pipeline-model-definition.jar!/org/jenkinsci/plugins/pipeline/modeldefinition/ModelInterpreter.groovy:620)
	at org.jenkinsci.plugins.pipeline.modeldefinition.ModelInterpreter.call(jar:file:/C:/Users/ikedam/.jenkins/plugins/pipeline-model-definition/WEB-INF/lib/pipeline-model-definition.jar!/org/jenkinsci/plugins/pipeline/modeldefinition/ModelInterpreter.groovy:65)
	at WorkflowScript.run(WorkflowScript:1)

It looks depend on the internal order of symbols:

  1. Test without copyartifact-1.39 -> succeeds (of course)
  2. Test after installing copyartifact-1.39 -> succeeds
  3. Test after rebooting Jenkins -> fail

@abayer
Copy link
Member

abayer commented Nov 6, 2017

@ikedam What? Argh! I could have sworn it was fixed! I must have missed some context for narrowing down the symbol scope...

@abayer
Copy link
Member

abayer commented Nov 6, 2017

@ikedam Augh, you're so very right. I verified that validation worked, not that the actual trigger definition worked. facepalm

@ikedam
Copy link
Member Author

ikedam commented Nov 8, 2017

The issue is fixed in pipeline-model-definition 1.2.4.
Thanks!

Creating a simple DataBoundConstructor will be fixed in another pull request JENKINS-47905.

@ikedam ikedam closed this Nov 8, 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
3 participants