Stop using Guice to inject `InputStep` & context parameters #69

jglick · 2022-01-03T23:40:45Z

Downstream of jenkinsci/workflow-step-api-plugin#81. Similar to #64 but should retain serial compatibility for steps running across the upgrade. Probably fixes jenkinsci/acceptance-test-harness#727.

basil · 2022-01-03T23:44:51Z

src/main/java/org/jenkinsci/plugins/workflow/support/steps/input/InputStepExecution.java

        } catch (IOException | InterruptedException | TimeoutException x) {
-            LOGGER.log(Level.WARNING, "failed to remove InputAction from " + run, x);
+            LOGGER.log(Level.WARNING, "failed to remove InputAction from " + getContext(), x);


Looks reasonable, but here and elsewhere where you call getContext() did you mean getRun()? That would make more sense to me as a direct translation of the old run variable.

(I see what you mean about GitHub not letting you comment on a single line without leaving a "review". I clicked "leave a single comment", yet now it appears as "reviewed 18 minutes ago.")

getRun() would be the more direct equivalent, but might throw exceptions. getContext() is good enough for logging purposes as its toString() will include an identification of the build: https://github.com/jenkinsci/workflow-cps-plugin/blob/a9433432b33c65f58d9e7b4dc4aa83249672eaf7/src/main/java/org/jenkinsci/plugins/workflow/cps/CpsStepContext.java#L596-L605 + https://github.com/jenkinsci/workflow-job-plugin/blob/969637446d3ba7ea2c1b26cb7ee1b9c44c6e4ebc/src/main/java/org/jenkinsci/plugins/workflow/job/WorkflowRun.java#L1003-L1005

Got it, sounds good then.

jglick · 2022-01-04T15:25:07Z

It would be prudent for me to also add a @LocalData test demonstrating that #64 is incompatible while this is compatible.

…step-plugin into guice

jglick · 2022-01-04T18:51:12Z

Without using AbstractStepExecutionImpl (like in #64) the new InputStepTest.serialForm fails as expected:

an exception which occurred:
	in field org.jenkinsci.plugins.workflow.cps.CpsThread.step
	in object org.jenkinsci.plugins.workflow.cps.CpsThread@3954e084
	in object of type org.jenkinsci.plugins.workflow.cps.CpsThread
	in object of type java.util.concurrent.ConcurrentSkipListMap
	in field org.jenkinsci.plugins.workflow.cps.CpsThreadGroup.threads
	in object org.jenkinsci.plugins.workflow.cps.CpsThreadGroup@7022d974
	in object of type org.jenkinsci.plugins.workflow.cps.CpsThreadGroup
Caused: java.io.InvalidClassException: org.jenkinsci.plugins.workflow.support.steps.input.InputStepExecution; Class does not extend stream superclass
	at org.jboss.marshalling.river.RiverUnmarshaller.doReadClassDescriptor(RiverUnmarshaller.java:1046)
	at …
	at org.jenkinsci.plugins.workflow.support.pickles.serialization.RiverReader$SandboxedUnmarshaller.lambda$readObject$0(RiverReader.java:250)
	at org.jenkinsci.plugins.scriptsecurity.sandbox.groovy.GroovySandbox.runInSandbox(GroovySandbox.java:237)
	at org.jenkinsci.plugins.workflow.support.pickles.serialization.RiverReader$SandboxedUnmarshaller.sandbox(RiverReader.java:237)
	at org.jenkinsci.plugins.workflow.support.pickles.serialization.RiverReader$SandboxedUnmarshaller.readObject(RiverReader.java:250)
	at org.jenkinsci.plugins.workflow.cps.CpsFlowExecution$2.onSuccess(CpsFlowExecution.java:793)
	at org.jenkinsci.plugins.workflow.cps.CpsFlowExecution$2.onSuccess(CpsFlowExecution.java:786)
	at …
	at org.jenkinsci.plugins.workflow.cps.CpsFlowExecution.loadProgramAsync(CpsFlowExecution.java:783)
	at org.jenkinsci.plugins.workflow.cps.CpsFlowExecution.onLoad(CpsFlowExecution.java:750)
	at org.jenkinsci.plugins.workflow.job.WorkflowRun.getExecution(WorkflowRun.java:691)
	at org.jenkinsci.plugins.workflow.job.WorkflowRun.onLoad(WorkflowRun.java:550)
	at hudson.model.RunMap.retrieve(RunMap.java:225)
	at hudson.model.RunMap.retrieve(RunMap.java:57)
	at jenkins.model.lazy.AbstractLazyLoadRunMap.load(AbstractLazyLoadRunMap.java:501)
	at jenkins.model.lazy.AbstractLazyLoadRunMap.load(AbstractLazyLoadRunMap.java:483)
	at jenkins.model.lazy.AbstractLazyLoadRunMap.getByNumber(AbstractLazyLoadRunMap.java:381)
	at hudson.model.RunMap.getById(RunMap.java:205)
	at org.jenkinsci.plugins.workflow.job.WorkflowRun$Owner.run(WorkflowRun.java:940)
	at org.jenkinsci.plugins.workflow.job.WorkflowRun$Owner.get(WorkflowRun.java:951)
	at org.jenkinsci.plugins.workflow.flow.FlowExecutionList$1.computeNext(FlowExecutionList.java:65)
	at org.jenkinsci.plugins.workflow.flow.FlowExecutionList$1.computeNext(FlowExecutionList.java:57)
	at com.google.common.collect.AbstractIterator.tryToComputeNext(AbstractIterator.java:143)
	at com.google.common.collect.AbstractIterator.hasNext(AbstractIterator.java:138)
	at org.jenkinsci.plugins.workflow.flow.FlowExecutionList$ItemListenerImpl.onLoaded(FlowExecutionList.java:178)
	at jenkins.model.Jenkins.<init>(Jenkins.java:1019)
	at …
Caused: java.io.IOException: Failed to load build state
	at …
Finished: FAILURE

Without using AbstractStepImpl it in fact passes, suggesting jenkinsci/workflow-step-api-plugin#81 was not strictly necessary. strings program.dat | uniq | sort records the type InputStep but not its supertype for some reason (whereas the supertype of InputStepExecution is recorded). Not sure what is going on there, but at any rate it seems safer to retain the original supertype.

jglick · 2022-01-04T19:00:29Z

$ strings src/test/resources/org/jenkinsci/plugins/workflow/support/steps/input/InputStepTest/serialForm/jobs/p/builds/1/program.dat | sort | uniq | fgrep Step
	>0org.jenkinsci.plugins.workflow.steps.StepContext
	>1org.jenkinsci.plugins.workflow.cps.CpsStepContext
	>2org.jenkinsci.plugins.workflow.steps.StepExecution
3><org.jenkinsci.plugins.workflow.support.steps.input.InputStep
	>9org.jenkinsci.plugins.workflow.support.DefaultStepContextG
	>Eorg.jenkinsci.plugins.workflow.support.steps.input.InputStepExecution
	>>org.jenkinsci.plugins.workflow.steps.AbstractStepExecutionImpl

Possibly because StepExecution implements Serializable, so all supertypes leading back to that are part of the serial form, whereas InputStep extends AbstractStepImpl implements Serializable so (in the absence of any inherited fields) the supertype of InputStep is not part of the serial form? I guess I could revert jenkinsci/workflow-step-api-plugin#81 (I know, I should have written the test first) and make InputStep extend Step for simplicity though I am not entirely confident that is safe. No strong opinion.

Stop using Guice to inject InputStep & context parameters

2d830f2

jglick added the bug label Jan 3, 2022

jglick requested review from basil and car-roll January 3, 2022 23:41

basil reviewed Jan 3, 2022

View reviewed changes

jglick mentioned this pull request Jan 3, 2022

WorkflowPluginTest#linearFlow is flakey jenkinsci/acceptance-test-harness#727

Open

Got an incremental deployment

e04bbbb

basil approved these changes Jan 4, 2022

View reviewed changes

basil mentioned this pull request Jan 4, 2022

Remove usages of Guice #64

Closed

jglick mentioned this pull request Jan 4, 2022

Allow AbstractStepImpl to still be subclassed for serial compatibility jenkinsci/workflow-step-api-plugin#81

Merged

jglick added 2 commits January 4, 2022 12:03

Merge branch 'master' of https://github.com/jenkinsci/pipeline-input-…

0cf9ca2

…step-plugin into guice

jenkinsci/workflow-step-api-plugin#81 released

e9b5c5c

jglick mentioned this pull request Jan 4, 2022

Make BuildWatcher behave better with @LocalData jenkinsci/jenkins-test-harness#377

Merged

jglick added 2 commits January 4, 2022 13:40

Added InputStepTest.serialForm passing against master

f3b863d

Merge commit 'f3b863df05880c48407c9aa2eaa16fc7ea299738' into guice

a060d36

jglick marked this pull request as ready for review January 4, 2022 18:51

jglick requested review from basil and dwnusbaum January 4, 2022 18:51

basil approved these changes Jan 7, 2022

View reviewed changes

car-roll approved these changes Jan 19, 2022

View reviewed changes

car-roll merged commit f27b0b8 into jenkinsci:master Jan 19, 2022

jglick deleted the guice branch January 19, 2022 19:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Stop using Guice to inject `InputStep` & context parameters #69

Stop using Guice to inject `InputStep` & context parameters #69

jglick commented Jan 3, 2022 •

edited

basil Jan 3, 2022

basil Jan 4, 2022

jglick Jan 4, 2022

basil Jan 4, 2022 •

edited

jglick commented Jan 4, 2022

jglick commented Jan 4, 2022

jglick commented Jan 4, 2022

Stop using Guice to inject InputStep & context parameters #69

Stop using Guice to inject InputStep & context parameters #69

Conversation

jglick commented Jan 3, 2022 • edited

basil Jan 3, 2022

Choose a reason for hiding this comment

basil Jan 4, 2022

Choose a reason for hiding this comment

jglick Jan 4, 2022

Choose a reason for hiding this comment

basil Jan 4, 2022 • edited

Choose a reason for hiding this comment

jglick commented Jan 4, 2022

jglick commented Jan 4, 2022

jglick commented Jan 4, 2022

Stop using Guice to inject `InputStep` & context parameters #69

Stop using Guice to inject `InputStep` & context parameters #69

jglick commented Jan 3, 2022 •

edited

basil Jan 4, 2022 •

edited