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

Removing uses of Guice #28

Merged
merged 2 commits into from Dec 1, 2016

Conversation

Projects
None yet
4 participants
@jglick
Copy link
Member

commented Nov 30, 2016


/** Placeholder for getRequiredContext. */
@StepContextParameter private transient Run<?,?> run;
@StepContextParameter private transient TaskListener listener;

This comment has been minimized.

Copy link
@jglick

jglick Nov 30, 2016

Author Member

One of the evil tricks that is no longer necessary.


private static final long serialVersionUID = 1L;

@Inject(optional=true) private transient SleepStep step;
@StepContextParameter private transient TaskListener listener;
private transient final SleepStep step;

This comment has been minimized.

Copy link
@jglick

jglick Nov 30, 2016

Author Member

Alternately could have kept just the result of step.unit.toMillis(step.time).

@reviewbybees

This comment has been minimized.

Copy link

commented Nov 30, 2016

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.

return getContext().get(TaskListener.class);
} catch (Exception x) {
LOGGER.log(Level.WARNING, null, x);
return TaskListener.NULL;

This comment has been minimized.

Copy link
@jglick

jglick Nov 30, 2016

Author Member

When using injection, there were two ways StepContext.get errors were handled: AbstractStepImpl.start would throw up the exception, which we could still do in a StepExecution constructor; and AbstractStepExecutionImpl.onResume would send the exception to StepContext.onFailure (but presumably leave the field null, without preventing the rest of an onResume override from running—perhaps the root cause of JENKINS-34021 and the still-open JENKINS-37486).

This idiom is more verbose but clearer—the step gets to decide what to do.

Alternately, we could change onResume to throw Exception. (Would still be binary-compatible.) FlowExecutionList.ItemListenerImpl.onLoaded would then need to catch exceptions and call StepContext.onFailure. Like AbstractStepExecutionImpl, this would rely on transient fields holding the contextual object; the improvement would be that a failure in StepContext.get would stop anything else in onResume (such as setupTimer here) from running.

This comment has been minimized.

Copy link
@jglick

jglick Dec 1, 2016

Author Member

Filed JENKINS-40161 to track the latter proposal.

@StepContextParameter private transient Launcher launcher;
@StepContextParameter private transient TaskListener listener;
@SuppressFBWarnings(value="SE_TRANSIENT_FIELD_NOT_RESTORED", justification="Only used when starting.")
private transient final SimpleBuildStep delegate;

This comment has been minimized.

Copy link
@jglick

jglick Nov 30, 2016

Author Member

Have the option of simplifying StepExecution state, especially in cases where the Step had only one field.

@abayer

abayer approved these changes Dec 1, 2016

Copy link
Member

left a comment

🐝

@stephenc

This comment has been minimized.

Copy link
Member

commented Dec 1, 2016

🐝

@jglick jglick merged commit f5aff4c into jenkinsci:master Dec 1, 2016

1 check passed

Jenkins This pull request looks good
Details

@jglick jglick deleted the jglick:guiceless-JENKINS-39134 branch Dec 1, 2016

jglick added a commit that referenced this pull request Dec 12, 2016


public static class Execution extends AbstractStepExecutionImpl {
public static class Execution extends StepExecution {

This comment has been minimized.

Copy link
@jglick

jglick Jan 9, 2017

Author Member

Likely to cause JENKINS-40909.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.