-
Notifications
You must be signed in to change notification settings - Fork 395
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
Implement propertyMissing method #299
base: master
Are you sure you want to change the base?
Implement propertyMissing method #299
Conversation
683593f
to
a08f1d5
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have a question regarding reverting some changes
src/main/groovy/com/lesfurets/jenkins/unit/declarative/AgentDeclaration.groovy
Show resolved
Hide resolved
@@ -14,24 +15,14 @@ abstract class GenericPipelineDeclaration { | |||
static <T> T createComponent(Class<T> componentType, @DelegatesTo(strategy = DELEGATE_FIRST) Closure<T> closure) { | |||
// declare componentInstance as final to prevent any multithreaded issues, since it is used inside closure | |||
final def componentInstance = componentType.newInstance() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think at the moment we create a new instance of whatever we should call our custom class loader.
the problem is createComponent
method is static so I'm not sure how to retrieve it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so no additional juggling with bindings are required in execute
neither with componentInstance
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm sorry i don't quite follow. Are you saying we want to get rid of the createComponent call alltogether? The only change i did in this MR is flip 2 vars around. Making closure the delegate and componentInstance the owner. So that the closure is evaluated before accessing the Declaration class methods/vars
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@stchar I tried simplifying the whole class loading.
The idea:
We only need stack interception for Script calls? (not the declaration setup calls, as far as i understand it)
The declaration classes get called to set properties as a sort of setup of the stack. When execute is called on the Declaration it uses the Script to get the correct depth and stack logging to work.
The parameters declaration now uses the binding of the closure (coming from PipelineTestHelper/BasePipelineTest)
A problem i ran into when trying to intercept the declaration classes is calling super methods. More specifically the super.execute call in NotDeclaration. The current MethodInterceptor seems unable to distinguish when to call NotDeclaration.execute vs super(/WhenDeclaration).execute. It seems to keep trying to intercept and invoke NotDeclaration.execute. This caused when_not_branch_master to fail.
So from that i tried just making sure the closures get bound to a Declaration delegate and the Declaration class delegates calls throug the script to get through the Stack interception
src/main/groovy/com/lesfurets/jenkins/unit/declarative/PostDeclaration.groovy
Outdated
Show resolved
Hide resolved
src/main/groovy/com/lesfurets/jenkins/unit/declarative/StageDeclaration.groovy
Outdated
Show resolved
Hide resolved
executeWith(delegate, { echo "Stage \"$name\" skipped due to earlier failure(s)" }) | ||
return | ||
} | ||
|
||
if (!when || when.execute(delegate)) { | ||
super.execute(delegate) | ||
// Set environment for stage |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would prefer not to copy-paste same code in different classes inheritance here is a good practice
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The stage should clear the environment/params after finishing so the next stage could inherit pipeline vars. But i guess we could implement the cleanup at GenericPipeline level
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I created a public static method to init and reset the environment props. This way we can share the logic, but choose when to call the parts.
We want to init the env in a stage, but only reset it after the stage is complete. Super.execute would have immediately cleared the stage custom env overrides.
a08f1d5
to
32dc9db
Compare
6681845
to
fd171b4
Compare
… adds unwanted properties) Remove GenericPipelineDeclaration implementation of getProperty. Instead implement propertyMissing method to fallback to env and params. Append test to use env var before pipeline closure Remove duplicate method executeOn (use executeWith instead) Remove name conflicts between JenkinsFile closures and Declaration fields
2e8d40d
to
e7360c8
Compare
e7360c8
to
78b8154
Compare
Undo extending all Declarations from GenericPipelineDeclaration (this adds unwanted properties)
Remove GenericPipelineDeclaration implementation of getProperty.
Instead implement propertyMissing method to fallback to env and params.
Append test to use env var before pipeline closure
Remove duplicate method executeOn (use executeWith instead)
Remove name conflicts between JenkinsFile closures and Declaration fields
Our JenkinsFile had this:
It seems the previous fix to handle looking into env. and params. does not work outside the pipeline closure. Therefore i looked for another way to fix this.
During this i noticed the effect extending GenericPipelineDeclaration has on the function printNonNullProperties of ObjectUtils.
So i undid the extending of PipelineDeclaration for declarations that should not be able to handle all those functions(agent, steps etc).