Skip to content

Conversation

car-roll
Copy link

@car-roll car-roll commented Nov 17, 2020

There are some conditions in which the NamedArgsAndClosure class is passed to other packages where it is not recognized. This fix here goes back to an earlier implementation in #370 where we create a subclass of UninstantiatedDescribable that stores interpolated strings. This should ensure that existing code that used to look for UninstantiatedDescribable can continue to work as expected.

Note: not sure what tests to add to reflect this problem. Did manual testing with the Gerrit plugin

…dArgsAndClosure as return value for invokeDescribable
@car-roll car-roll requested a review from dwnusbaum November 17, 2020 20:01
@car-roll car-roll changed the title [NGPIPELINE-1537] Replace use of NamedArgsAndClosure as return value for invokeDescribable [JENKINS-64185] Replace use of NamedArgsAndClosure as return value for invokeDescribable Nov 17, 2020
@Costallat
Copy link

Tested here and it worked again, tested manually triggering the job and also using Gerrit.

Copy link

@Costallat Costallat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested here, and it worked

// UninstantiatedDescribable is set when the associated symbol is being built as the parameter of a step to be invoked.
UninstantiatedDescribable uninstantiatedDescribable = null;

private NamedArgsAndClosure(Map<?,?> namedArgs, Closure body, @Nonnull Set<String> foundInterpolatedStrings) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@car-roll Do we need to do any cleanup in NamedArgsAndClosure now? I guess we should remove implements Serializable and remove "The instance is returned to the Groovy program ..." from the Javadoc.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Technically the whole paragraph might have to go, right? Because we are now using the UninstantiatedDescribable again. Maybe that paragraph should go to UninstantiatedDescribableWithInterpolation?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah you are right, everything other than "This class holds the argument map and optional body of the step that is to be invoked." is no longer accurate. Feel free to move it to the new class if you want.

import java.util.Map;
import java.util.Set;

public class UninstantiatedDescribableWithInterpolation extends UninstantiatedDescribable {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should restrict this class (some imports will need to be added along with this suggestion):

Suggested change
public class UninstantiatedDescribableWithInterpolation extends UninstantiatedDescribable {
@Restricted(NoExternalUse.class)
public class UninstantiatedDescribableWithInterpolation extends UninstantiatedDescribable {

@car-roll car-roll closed this Nov 18, 2020
@car-roll car-roll reopened this Nov 18, 2020
@car-roll car-roll merged commit f7efd87 into jenkinsci:master Nov 18, 2020
@car-roll car-roll deleted the uninstantiated-describable-with-interpolation branch November 18, 2020 22:36
edwardkerry added a commit to alphagov/govuk-jenkinslib that referenced this pull request Jul 12, 2021
…uesMap

Version v2.86 workflow-cps plugin extended the UninstantiatedDescribable class from
the structs plugin to store Groovy interpolated strings.

This commit ensures we are able to handle both classes when building in Jenkins.

jenkinsci/workflow-cps-plugin#399
https://github.com/jenkinsci/workflow-cps-plugin/blob/1ff8b0f5ec3479f27a7b23cab892b312a9d2d78a/src/main/java/org/jenkinsci/plugins/workflow/cps/UninstantiatedDescribableWithInterpolation.java
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants