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-34005 - Convert WorkflowJob.triggers to a JobProperty #9

Merged
merged 16 commits into from Jul 28, 2016

Conversation

Projects
None yet
4 participants
@abayer
Copy link
Member

commented Jul 13, 2016

JENKINS-34005

  • Add new WorkflowTriggersJobProperty
  • Testing adding/removing/setting triggers
  • Testing migration from old style.
  • Verify UI behavior including config round tripping.
  • Verify comparison of behavior with TriggerStartTest for AbstractProject in core.
  • properties step test over in workflow-multibranch-plugin (see jenkinsci/workflow-multibranch-plugin#20)

cc @reviewbybees esp @jglick

WorkflowTriggersJobProperty triggerProp = getTriggersJobProperty();
if (triggerProp != null) {
for (Trigger t : triggerProp.getTriggers()) {
t.stop();

This comment has been minimized.

Copy link
@jglick

jglick Jul 13, 2016

Member

Seems antimodular. This should be done in WorkflowTriggersJobProperty itself.

This comment has been minimized.

Copy link
@abayer

abayer Jul 13, 2016

Author Member

Yeah, realizing that.

PipelineTriggersJobProperty triggerProp = getTriggersJobProperty();
if (triggerProp != null) {
for (Trigger t : triggerProp.getTriggers()) {
t.stop();

This comment has been minimized.

Copy link
@jglick

jglick Jul 13, 2016

Member

Antimodular. Do this in WorkflowTriggersJobProperty.reconfigure.

// Since we would have already started them in setTriggers...
else {
for (Trigger t : getTriggersJobProperty().getTriggers()) {
t.start(this, Items.currentlyUpdatingByXml());

This comment has been minimized.

Copy link
@jglick

jglick Jul 13, 2016

Member

Do this in WorkflowTriggersJobProperty.setJob.

This comment has been minimized.

Copy link
@abayer

abayer Jul 13, 2016

Author Member

setJob? Do you mean setOwner?

This comment has been minimized.

Copy link
@jglick

jglick Jul 13, 2016

Member

Whatever it is.

}

for (Trigger t2 : triggerProp.getTriggers()) {
t2.start(this, true);

This comment has been minimized.

Copy link
@jglick

jglick Jul 13, 2016

Member

Could more simply edit the existing property and reconfigure it. Or at least extract the stop/update/start code into a reusable method.

This comment has been minimized.

Copy link
@abayer

abayer Jul 13, 2016

Author Member

Yeah, gonna do the latter.

if (old != null) {
old.stop();
}
trigger.start(this, true);

This comment has been minimized.

Copy link
@jglick

jglick Jul 13, 2016

Member

Ditto.

PipelineTriggersJobProperty triggerProp = getTriggersJobProperty();
if (triggerProp != null) {
for (Trigger<?> trigger : triggerProp.getTriggers()) {
actions.addAll(trigger.getProjectActions());

This comment has been minimized.

Copy link
@jglick

jglick Jul 13, 2016

Member

Could probably delete this whole override, and replace with a TransientActionFactory in WorkflowTriggersJobProperty.

This comment has been minimized.

Copy link
@abayer

abayer Jul 14, 2016

Author Member

Would that mean changing to super.getAllActions(), actually?

@@ -483,7 +551,8 @@ public void replaceAction(Action a) {
}

@Override public SCMTrigger getSCMTrigger() {
return triggers.get(SCMTrigger.class);
// TODO: This seems fairly weird to me and I'm not 100% sure why it works.
return (SCMTrigger) getTriggers().get(getDescriptorByName("SCMTrigger"));

This comment has been minimized.

Copy link
@jglick

jglick Jul 13, 2016

Member

Do not do it this way.

This comment has been minimized.

Copy link
@abayer

abayer Jul 13, 2016

Author Member

So what way should I do it?

This comment has been minimized.

Copy link
@jglick

jglick Jul 13, 2016

Member

Just search the list for instanceof SCMTrigger? There is a helper function somewhere in Util IIRC.


// TODO: See if we actually need to check this or can assume it's always non-null
if (form != null) {
thisProp = (PipelineTriggersJobProperty)getDescriptor().newInstance(req, form);

This comment has been minimized.

Copy link
@abayer

abayer Jul 13, 2016

Author Member

So it turns out this straight-up doesn't work. Yay! I've verified that the form content is correct (with my current config.jelly), but by the time it gets to the PipelineTriggersJobProperty constructor, it's getting a null list of Triggers. So something in the binding is not working with my approach. Grr! Am I forgetting something obvious?

This comment has been minimized.

Copy link
@jglick

jglick Jul 13, 2016

Member

Do not call newInstance I guess. Use req.bindJSON(this, form) or something along those lines. Or do something a bit lower-level, pulling the list out of the form and using DescriptorList methods IIRC to data-bind it to a list.

This comment has been minimized.

Copy link
@abayer

abayer Jul 13, 2016

Author Member

Okiedokie - will try those when I get home.

abayer added some commits Jul 14, 2016

So this generally works now.
That said, it needs tweaks - the UI needs work, since currently it
just shows up in the job properties, with no break between the
triggers and subsequent properties, which is not a good
experience. And while I've verified by hand that triggers are
successfully stopped, started, persisted through a config round trip,
etc, actual tests are still very much needed.

That said, this *does* generally work!
First working test.
Fixed roundtriping in general too. Sorta.
@abayer

This comment has been minimized.

Copy link
Member Author

commented Jul 14, 2016

So as it turns out, calling startTriggers() in PipelineTriggersJobProperty.setOwner causes...chaos. Way too many calls to startTriggers() with confusing variants on the newInstance flag. The only way I can replicate behavior from TriggerStartTest in core is to remove the call from setOwner and put it back in WorkflowJob.onLoad, so, well, I'm doing that.

@reviewbybees

This comment has been minimized.

Copy link

commented Jul 14, 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.

for (Trigger t : triggers) {
t.start(this, Items.currentlyUpdatingByXml());
if (triggers != null && !triggers.isEmpty()) {
setTriggers(triggers.toList());

This comment has been minimized.

Copy link
@jglick

jglick Jul 26, 2016

Member

🐛 this is going to call startTriggers(true) whereas it should usually be false (unless we are currentlyUpdatingByXml).

This comment has been minimized.

Copy link
@jglick

jglick Jul 26, 2016

Member

Only affects the one-time load after upgrading this plugin, but still—for some plugins the value of newInstance may be important.

This comment has been minimized.

Copy link
@abayer

abayer Jul 26, 2016

Author Member

Ok, will change to use currentlyUpdatingByXml here.

This comment has been minimized.

Copy link
@abayer

abayer Jul 26, 2016

Author Member

More accurately, I changed setTriggers(list)'s call to startTriggers to startTriggers(Items.currentlyUpdatingByXml).

This comment has been minimized.

Copy link
@jglick

jglick Jul 26, 2016

Member

But that will be wrong for calls from anywhere but onLoad, where we expect true, and currentlyUpdatingByXml will likely be false.

if (old != null) {
old.stop();
triggers.remove(old);
public PipelineTriggersJobProperty getTriggersJobProperty() {

This comment has been minimized.

Copy link
@jglick

jglick Jul 26, 2016

Member

Could be private I think.

This comment has been minimized.

Copy link
@abayer

abayer Jul 26, 2016

Author Member

Done.

This comment has been minimized.

Copy link
@abayer

abayer Jul 26, 2016

Author Member

Actually, no, it can't - needed in the TransientActionFactory for PipelineTriggersJobProperty.

return null;
}

public void stopTrigggers() {

This comment has been minimized.

Copy link
@jglick

jglick Jul 26, 2016

Member

🐛 typo (would be an 🐜 if made package-private)

This comment has been minimized.

Copy link
@abayer

abayer Jul 26, 2016

Author Member

Fixed.

<j:jelly xmlns:j="jelly:core" xmlns:st="jelly:stapler" xmlns:d="jelly:define" xmlns:l="/lib/layout" xmlns:t="/lib/hudson" xmlns:f="/lib/form">
<!-- build triggers config pane -->
<j:invokeStatic var="triggers" className="hudson.triggers.Trigger" method="for_">
<j:arg value="${it}" type="hudson.model.Item" />

This comment has been minimized.

Copy link
@jglick

jglick Jul 26, 2016

Member

I tend to prefer encapsulating such logic in a method in the DescriptorImpl so the Jelly view stays short and simple.

This comment has been minimized.

Copy link
@abayer

abayer Jul 26, 2016

Author Member

Makes sense - this was copy-pasted from config-triggers.jelly, or I would have done something similar already. =)

This comment has been minimized.

Copy link
@abayer

abayer Jul 26, 2016

Author Member

Actually, off the top of my head (and admittedly at the end of the day, so my head may not be all there), I'm not sure of an easy way to encapsulate it in WorkflowJob.DescriptorImpl, so I'm gonna leave it as is at least for tonight.


originalProp.stopTriggers();

triggerProp.startTriggers(Items.currentlyUpdatingByXml());

This comment has been minimized.

Copy link
@jglick

jglick Jul 26, 2016

Member

Maybe this should just be deleted now, since onLoad is calling it already?

This comment has been minimized.

Copy link
@jglick

jglick Jul 26, 2016

Member

Or rather, could be moved into submit.

This comment has been minimized.

Copy link
@abayer

abayer Jul 26, 2016

Author Member

submit is already (effectively) calling startTriggers via PipelineTriggersJobProperty.reconfigure.

public static class MockTrigger extends Trigger<Item> {

public transient List<Boolean> calls = new ArrayList<Boolean>();
public transient boolean isStarted = false;

This comment has been minimized.

Copy link
@jglick

jglick Jul 26, 2016

Member

Might be more convincing for these to be replaced with a single

/** elements are true or false for {@code start}, null for {@code stop} */
public static List<Boolean> startsAndStops = new ArrayList<>();

so that you can demonstrate that the correct number and kind of calls are made on both old and new instances: that there are not, for example, temporary Triggers constructed, started, then discarded without being stopped.

This would be closer to the real behavior of Triggers overriding these methods, which tend to do things like register webhooks etc. with some global effect, where you want to ensure that calls are properly paired.

This comment has been minimized.

Copy link
@abayer

abayer Jul 26, 2016

Author Member

Good idea. Gonna do that and switch to deploying a test jar here for reuse over in jenkinsci/workflow-multibranch-plugin#20

This comment has been minimized.

Copy link
@abayer

abayer Jul 26, 2016

Author Member

aaaand good call - there's some weirdness with start/stop!

assertEquals(TimerTrigger.class, fromProp.getClass());

Trigger fromJob = p.getTriggers().get(fromProp.getDescriptor());
assertEquals(fromProp, fromJob);

This comment has been minimized.

Copy link
@jglick

jglick Jul 26, 2016

Member

This also needs to be measuring the start/stop calls. I suspect that in your current code, start is called twice.

This comment has been minimized.

Copy link
@abayer

abayer Jul 26, 2016

Author Member

Will check - it's definitely only getting called once in loadCallsStartFalse and submitCallsStartTrue or they'd be failing on the t.calls.toString() assert.

Reworking MockTrigger and start/stop logic.
MockTrigger is now used by workflow-multibranch as well, so deploy the
tests jar too.
if (req.getParameter("hasCustomQuietPeriod") != null) {
quietPeriod = Integer.parseInt(req.getParameter("quiet_period"));
} else {
quietPeriod = null;
}
triggers.rebuild(req, json, Trigger.for_(this));
for (Trigger t : triggers) {
t.start(this, true);

This comment has been minimized.

Copy link
@jglick

jglick Jul 28, 2016

Member

Now being handled in reconfigure IIUC.

This comment has been minimized.

Copy link
@abayer

abayer Jul 28, 2016

Author Member

Yes.

}

@Extension
@Symbol("pipelineTriggers")

This comment has been minimized.

Copy link
@jglick

jglick Jul 28, 2016

Member

@kohsuke it is a pity that the symbol must be unique amongst all JobPropertys, even though isApplicable (inherited and using the WorkflowJob type parameter) means that for a given job or job type just triggers would suffice. But I guess fixing that could be complicated: resolveClass in structs would need to be supplied with a filter predicate to apply to candidate extensions.


@DataBoundConstructor
public PipelineTriggersJobProperty(List<Trigger<?>> triggers) {
// Defensive handling of when we get called via {@code Descriptor.newInstance} with no form data.

This comment has been minimized.

Copy link
@jglick

jglick Jul 28, 2016

Member

Unfortunately Jenkins’ crazy form binding code often uses different JSON data types (null vs. object vs. array) for lists with zero, one, or multiple elements, resp.

MockTrigger t = new MockTrigger();
p.addTrigger(t);
p.save();
p = (WorkflowJob) Items.load(p.getParent(), p.getRootDir());

This comment has been minimized.

Copy link
@jglick

jglick Jul 28, 2016

Member

🐜 More realistic simulations would include:

  • use @LocalData with a config.xml representing an existing trigger
  • add as above, but use RestartableJenkinsRule and do this test after the restart
  • use AbstractItem.doReload
assertNotNull(triggerProp);

assertEquals(2, triggerProp.getTriggers().size());
assertEquals(2, p.getTriggers().size());

This comment has been minimized.

Copy link
@jglick

jglick Jul 28, 2016

Member

Seems like it would be simpler to just assert that these two lists are equal, then check each element once?

assertEquals(1, roundTripWithTrigger.getTriggers().size());
List<Trigger<?>> modTriggers = new ArrayList<>(roundTripWithTrigger.getTriggers().values());

assertEquals(MockTrigger.class, modTriggers.get(0).getClass());

This comment has been minimized.

Copy link
@jglick

jglick Jul 28, 2016

Member

🐜 would be nice to include some trigger with configuration, such as TimerTrigger, in the list and verify that this configuration got correctly round-tripped by f:descriptorList.

@jglick

This comment has been minimized.

Copy link
Member

commented Jul 28, 2016

There are a couple of places where I think the tests could be improved, but 🐝 and congratulations on your patience with this slog!

@abayer

This comment has been minimized.

Copy link
Member Author

commented Jul 28, 2016

@jglick So given said slog, I'd say let's merge it as is and then I'll do a followup to address the 🐜s?

@jglick

This comment has been minimized.

Copy link
Member

commented Jul 28, 2016

Sure.

@jglick jglick merged commit 5ac7e97 into jenkinsci:master Jul 28, 2016

1 check passed

Jenkins This pull request looks good
Details
@KostyaSha

This comment has been minimized.

Copy link
Member

commented on 28dc6a6 Sep 22, 2016

Thanks for making hard debugging in repository

@KostyaSha

This comment has been minimized.

Copy link
Member

commented Sep 22, 2016

So since this version my triggers are broken.

@KostyaSha

This comment has been minimized.

Copy link
Member

commented Sep 22, 2016

Why design is screwed now for such wide installed plugin?

@jglick

This comment has been minimized.

Copy link
Member

commented Sep 22, 2016

If there seems to be a regression, please file details to reproduce in a minimal demo trigger and probably @abayer will take a look.

@KostyaSha

This comment has been minimized.

Copy link
Member

commented Sep 22, 2016

@jglick yes, i also reproducing. Just don't understand why such hacking is needed for fresh plugin. Multibranch may be designed in any way as it still new plugin.

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

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.