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-50665] - nonCPS.exec() wrapper + Support of argument parameter filtering #218

Open
wants to merge 9 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@oleg-nenashev
Copy link
Member

oleg-nenashev commented Apr 9, 2018

Sorry for integrating several changes in a single PR, I am just testing a fix for jenkinsci/pipeline-utility-steps-plugin#46 in Pipeline Utility Steps. It will act as a downstream PR

  • - Run tests with 2.107.1
  • - JENKINS-50665 - introduce new ArgumentsActionFilteringStepDescriptor interface, which allow tweaking persisted parameters if needed
  • - JENKINS-50670 - demonstrate the JEP-200 error propagation mistake I have noticed during the implementation

@reviewbybees @rsandell @abayer @svanoort @jglick

@oleg-nenashev oleg-nenashev requested review from jglick, svanoort and abayer Apr 9, 2018


@NonCPS
<V> V exec(Closure<V> body) {
body()

This comment has been minimized.

Copy link
@jglick

jglick Apr 9, 2018

Member

Does this actually work? I cannot imagine how it could. @NonCPS is applied lexically to compilation of this method source. It has no effect on the CPS transform of the body closure. I think you only think it works because your printTimestamp method returns true from StepExecution.start and thus is synchronous (would be more simply written using SynchronousStepExecution, and thus the CPS transformation status of the body is largely irrelevant. If you add, say, a sleep 1 statement inside the closure I expect you will see it blow up, or rather (pending #211) quietly behave strangely.

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev Apr 9, 2018

Author Member

Will check it. in jenkinsci/pipeline-utility-steps-plugin#46 a complex wrapper with multiple steps does not fail after such patch, but I agree it may be also not a case in real.

This comment has been minimized.

Copy link
@svanoort

svanoort Apr 9, 2018

Member

Yeah, I have really mixed feelings on this.

As a whole, the NonCPS mechanism is NOT good about advertising when it has been abused and failing appropriately -- I think @jglick might have some in-progress work that resolves that though?

[JENKINS-50665] - Remove the NonCPS wrapper from the pull request.
As @jglick says, it likely does not work anyway. To be tested later

@oleg-nenashev oleg-nenashev changed the title [JENKINS-50665, JENKINS-50666] - nonCPS.exec() wrapper + Support of argument parameter filtering [JENKINS-50665] - nonCPS.exec() wrapper + Support of argument parameter filtering Apr 9, 2018

@oleg-nenashev

This comment has been minimized.

Copy link
Member Author

oleg-nenashev commented Apr 9, 2018

@jglick I have removed NonCPS wrapper from the scope for now

@jglick

This comment has been minimized.

Copy link
Member

jglick commented Apr 13, 2018

Does #220 not handle this?

@svanoort

This comment has been minimized.

Copy link
Member

svanoort commented Apr 13, 2018

@jglick Parts yes, parts no -- I think the ability @oleg-nenashev proposes to let steps customize filtering of what they persist is actually quite interesting and potentially useful completely aside from the original issue. Maybe not quite the right implementation for general use, but I want to mull a bit to think about where this might be applied.

@oleg-nenashev

This comment has been minimized.

Copy link
Member Author

oleg-nenashev commented Apr 14, 2018

Anyway, I will rework jenkinsci/pipeline-utility-steps-plugin#46 to NOT depend on this PR

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.