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

[FIXED JENKINS-40921][FIXED JENKINS-33020] Only display triggers that make sense for a folder computation #89

Merged
merged 3 commits into from Mar 2, 2017

Conversation

@stephenc
Copy link
Member

commented Mar 1, 2017

@reviewbybees

This comment has been minimized.

Copy link

commented Mar 1, 2017

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.

@stephenc stephenc force-pushed the stephenc:jenkins-40921 branch from 3879ca8 to 9c378be Mar 1, 2017

@jglick
jglick approved these changes Mar 1, 2017
@@ -342,6 +342,17 @@ protected final String getSuccessfulDestination() {
return triggers.toMap();
}

public List<TriggerDescriptor> getTriggerDescriptors() {

This comment has been minimized.

Copy link
@jglick

jglick Mar 1, 2017

Member
@Restricted(DoNotUse.class)
List<TriggerDescriptor> result = new ArrayList<TriggerDescriptor>();
for (TriggerDescriptor d: Trigger.for_(this)) {
if (d instanceof TimerTrigger.DescriptorImpl) {
continue;

This comment has been minimized.

Copy link
@jglick

jglick Mar 1, 2017

Member

Arguably better done using DescriptorVisibilityFilter, so that it would apply automatically to any other UIs offering Triggers here.

This comment has been minimized.

Copy link
@stephenc

stephenc Mar 1, 2017

Author Member

I am assuming that core will add DescriptorVisibilityFilter support to Trigger.for_(Item) in which case this is just a second level filtering

This comment has been minimized.

Copy link
@jglick

jglick Mar 1, 2017

Member

Not to for_, since that operates without context. Rather you would have to call h.filterDescriptors when invoking descriptorList. Or use a higher-level control which does this automatically, if there is one.

<p:config-trigger/>
<f:descriptorList title="${%triggers(it.computation.displayName)}"
descriptors="${it.triggerDescriptors}"
instances="${it.triggers}"/>

This comment has been minimized.

Copy link
@jglick

jglick Mar 1, 2017

Member

I think this only actually works by accident in config-trigger.jelly. Better style is to use the field attribute.

Either way, you need a configRoundtrip test verifying that the databinding code is correct.

This comment has been minimized.

Copy link
@stephenc

stephenc Mar 1, 2017

Author Member

actually it works because it is seriously old-school binding!

@jglick
jglick approved these changes Mar 1, 2017
public List<TriggerDescriptor> getTriggerDescriptors() {
// TODO remove this once core has support for DescriptorVisibilityFilter on Trigger.for_(Item)

This comment has been minimized.

Copy link
@jglick

jglick Mar 1, 2017

Member

As above, that is a misrepresentation of what I said.

  • for_ should not call DescriptorVisibilityFilter
  • merely removing this would revert the fix; you would need to add a DescriptorVisibilityFilter implementation

This comment has been minimized.

Copy link
@stephenc

stephenc Mar 2, 2017

Author Member

yes, we would need to add a descriptor visibility filter as part of removing this method

@stephenc stephenc merged commit 0d35973 into jenkinsci:master Mar 2, 2017

1 check passed

Jenkins This pull request looks good
Details

@stephenc stephenc deleted the stephenc:jenkins-40921 branch Mar 2, 2017

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