Classing by mchugh19 · Pull Request #40 · jenkinsci/saltstack-plugin

mchugh19 · 2016-10-29T23:45:21Z

While this code implements better utilization of classes which better supports the pipeline moudule, it also doesn't properly run.

When running a pipeline containing a saltstack API cal, I get the response:

1) Could not find a suitable constructor in com.waytta.clientinterface.LocalClient$DescriptorImpl. Classes must have either one (and only one) constructor annotated with @Inject or a zero-argument constructor that is not private.
  at com.waytta.clientinterface.LocalClient$DescriptorImpl.class(LocalClient.java:45)
  while locating com.waytta.clientinterface.LocalClient$DescriptorImpl

1 error
    at com.google.inject.internal.InjectorImpl.getProvider(InjectorImpl.java:1031)
    at com.google.inject.internal.InjectorImpl.getProvider(InjectorImpl.java:990)
    at com.google.inject.internal.InjectorImpl.getInstance(InjectorImpl.java:1040)
    at jenkins.ProxyInjector.getInstance(ProxyInjector.java:98)
    at org.jenkinsci.plugins.structs.SymbolLookup.findDescriptor(SymbolLookup.java:101)
    at org.jenkinsci.plugins.workflow.cps.DSL.invokeMethod(DSL.java:128)
    at org.jenkinsci.plugins.workflow.cps.CpsScript.invokeMethod(CpsScript.java:108)
    at groovy.lang.MetaClassImpl.invokeMethodOnGroovyObject(MetaClassImpl.java:1280)
    at groovy.lang.MetaClassImpl.invokeMethod(MetaClassImpl.java:1174)
    at groovy.lang.MetaClassImpl.invokeMethod(MetaClassImpl.java:1024)
    at org.codehaus.groovy.runtime.callsite.PogoMetaClassSite.call(PogoMetaClassSite.java:42)
    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:16)
    at WorkflowScript.run(WorkflowScript:3)
    at ___cps.transform___(Native Method)
    at com.cloudbees.groovy.cps.impl.ContinuationGroup.methodCall(ContinuationGroup.java:48)
    at com.cloudbees.groovy.cps.impl.FunctionCallBlock$ContinuationImpl.dispatchOrArg(FunctionCallBlock.java:109)
    at com.cloudbees.groovy.cps.impl.FunctionCallBlock$ContinuationImpl.fixArg(FunctionCallBlock.java:82)
    at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
    at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
    at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
    at java.lang.reflect.Method.invoke(Method.java:498)
    at com.cloudbees.groovy.cps.impl.ContinuationPtr$ContinuationImpl.receive(ContinuationPtr.java:72)
    at com.cloudbees.groovy.cps.impl.CollectionLiteralBlock$ContinuationImpl.dispatch(CollectionLiteralBlock.java:55)
    at com.cloudbees.groovy.cps.impl.CollectionLiteralBlock$ContinuationImpl.item(CollectionLiteralBlock.java:45)
    at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
    at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
    at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
    at java.lang.reflect.Method.invoke(Method.java:498)
    at com.cloudbees.groovy.cps.impl.ContinuationPtr$ContinuationImpl.receive(ContinuationPtr.java:72)
    at com.cloudbees.groovy.cps.impl.ConstantBlock.eval(ConstantBlock.java:21)
    at com.cloudbees.groovy.cps.Next.step(Next.java:58)
    at com.cloudbees.groovy.cps.Continuable.run0(Continuable.java:154)
    at org.jenkinsci.plugins.workflow.cps.CpsThread.runNextChunk(CpsThread.java:163)
    at org.jenkinsci.plugins.workflow.cps.CpsThreadGroup.run(CpsThreadGroup.java:324)
    at org.jenkinsci.plugins.workflow.cps.CpsThreadGroup.access$100(CpsThreadGroup.java:78)
    at org.jenkinsci.plugins.workflow.cps.CpsThreadGroup$2.call(CpsThreadGroup.java:236)
    at org.jenkinsci.plugins.workflow.cps.CpsThreadGroup$2.call(CpsThreadGroup.java:224)
    at org.jenkinsci.plugins.workflow.cps.CpsVmExecutorService$2.call(CpsVmExecutorService.java:63)
    at java.util.concurrent.FutureTask.run(FutureTask.java:266)
    at hudson.remoting.SingleLaneExecutorService$1.run(SingleLaneExecutorService.java:112)
    at jenkins.util.ContextResettingExecutorService$1.run(ContextResettingExecutorService.java:28)
    at java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:511)
    at java.util.concurrent.FutureTask.run(FutureTask.java:266)
    at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1142)
    at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:617)
    at java.lang.Thread.run(Thread.java:745)

Thanks!

jglick · 2016-10-31T13:23:33Z

src/main/java/com/waytta/clientinterface/LocalClient.java

Avoid using @Extension on fields. Put it on the DescriptorImpl type.

jglick · 2016-10-31T13:23:50Z

src/main/java/com/waytta/clientinterface/LocalClient.java

+    public static final class DescriptorImpl extends BasicClientDescriptor {
+        private DescriptorImpl(Class<? extends BasicClient> clazz) {
+            super(clazz);
+        }


Delete and use the default no-arg constructor.

This is one of things I'm a bit confused by. Are you suggesting to change it to something like the following?

public static class DescriptorImpl extends BasicClientDescriptor { public DescriptorImpl() { super(LocalClient.class); }

jglick · 2016-10-31T13:25:14Z

src/main/java/com/waytta/clientinterface/LocalClient.java

Use primitives. For the actual fields, use primitives unless you really need to handle null values, typically only necessary for compatibly upgrading from an earlier version lacking the field, in which case readResolve would set a non-null value.

jglick · 2016-10-31T13:26:10Z

src/main/java/com/waytta/clientinterface/LocalClient.java

If there is a sensible default for any parameter, use @DataBoundSetter and delete from the @DataBoundConstructor. Guide

Done. Moved both jobPollTime and blockbuild to @DataBoudnSetter. Left the @DataBoundConstructor for target and targetType as it is required.

jglick · 2016-10-31T13:27:06Z

src/main/resources/com/waytta/clientinterface/LocalBatchClient/config.jelly

You do not need any of this. Just use f:select and have the descriptor supply choices.

jglick · 2016-10-31T13:29:18Z

src/main/java/com/waytta/clientinterface/BasicClient.java

-    public String getTargetType() {
-        return targetType;
+public class BasicClient implements ExtensionPoint, Describable<BasicClient> {
+	public Descriptor<BasicClient> getDescriptor() {


Just extend AbstractDescribableImpl, but @Override this method to cast the super value to BasicClientDescriptor.

jglick · 2016-10-31T13:29:38Z

src/main/java/com/waytta/clientinterface/BasicClient.java

-        this.blockBuild = blockBuild;
-    }
+    public static class BasicClientDescriptor extends Descriptor<BasicClient> {
+        public BasicClientDescriptor(Class<? extends BasicClient> clazz) {


Add a no-arg constructor.

jglick · 2016-10-31T13:29:49Z

src/main/java/com/waytta/clientinterface/BasicClient.java

Delete, this class should be abstract.

jglick · 2016-10-31T13:31:21Z

src/main/resources/com/waytta/clientinterface/LocalClient/help-target.html

Recommend target="_blank" to avoid clobbering a Jenkins configuration page.

Great catch. Done

jglick · 2016-10-31T13:31:40Z

src/main/java/com/waytta/clientinterface/LocalClient.java

+
+        @Override
+        public String getDisplayName() {
+        	return "local";


Conventionally should be capitalized, unless this is intended to match a code word in some external system.

Yep. This is the value passed along to the rest api

…job doens't crash. Pipeline version still doesn't output however :(

mchugh19 · 2016-10-31T22:50:03Z

Thanks for all your help @jglick! It looks like things generally work. Yay! But there's a few more things to do to be a good citizen with pipeline.

Are there recommendations for how to support hiding jelly fields and changing jelly help files when in pipeline mode? Some things that are done for freestyle projects are not necessary for pipeline.
Is there a good way of getting the default value of global.conf's pollTime into the jobPollTime default of LocalClient's config.jelly? Before this was a separate class, I could get the descriptor of SaltAPIBuilder.java. But now that LocalClient is its own class, I can't just have it's config.jelly use default="${descriptor.getPollTime()}" as that now refers to LocalClient's descriptor and not SaltAPIBuilder's.
How should the plugin send output? Currently, the plugin uses listener.getLogger().println() to display output. However this does not seem to be saved by

def saltoutput = salt arguments: 'ls -la', authtype: 'pam', clientInterface: local(blockbuild: false, jobPollTime: 5, target: userInput['target'], targetType: 'glob'), credentialsId: 'a3d814c2-84ed-4752-94a8-271791bb5375', function: 'cmd.run', kwarguments: userInput['kwarguments'], servername: 'http://localhost:8000', target: 'master', targettype: 'glob'
// output displayed in the build console here, even though it should have been saved into variable

echo ("Salt outputted ${saltoutput}")       // this creates "Salt outputted null"

Christian McHugh added 5 commits October 27, 2016 22:50

begin to use classes

63ab56c

unsuccessful attempt so far to more properly use classes

3e1fa6a

unsuccessful attempt so far to more properly use classes

a56ef56

got things working. Just need to add descriptors

0c6c3b4

functional class implementation

4c3666a

mchugh19 added the help wanted label Oct 29, 2016

jglick suggested changes Oct 31, 2016

View reviewed changes

Properly handles client classes. Now classic job works, and pipeline …

4af1330

…job doens't crash. Pipeline version still doesn't output however :(

Christian McHugh added 3 commits November 2, 2016 19:39

Move from hardcoding dropdown in jelly to a single descriptor

b32caa6

delete old failing tests

5cd4305

correct databoundconstructor for the new classes

0fcbbaa

mchugh19 merged commit 1f29d27 into refactoring Nov 2, 2016

mchugh19 deleted the classing branch November 2, 2016 20:19

Uh oh!

Conversation

mchugh19 commented Oct 29, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mchugh19 commented Oct 31, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Comments