Revisit groovy delegates and property/method lookup #289
Conversation
|
Please remove all indentation changes in this branch, as they make this PR harder to review. I agree that there is some inconsistency in the codebase, but we need a better approach to solving it. This is already on the radar, please see #237. |
|
I think i removed the whitespace changes. This change works for properties, but seems not to be effective for methods yet. |
|
Should also fix #280 with latest commit |
|
Sorry for all the commits. I was having issues with shelved changes and different changelists with reverting and such. Tests all ran successfully locally. I saw whitespace changes still in ParametersDeclaration which i can remove if needed. |
|
How would i go about this? c74addd Should have been part of Remove owner param in Create component. I'm trying with intelliJ as well as TortoiseGit but i don't usually update branch history |
Willem1987
force-pushed
the
update-delegate-handling
branch
from
c8d6e90
to
0893a20
Compare
|
It is now 1 commit |
|
@Willem1987 Great, thanks. I'll try to take a look at this PR (and the others) today or tomorrow. |
src/main/groovy/com/lesfurets/jenkins/unit/declarative/AnyOfDeclaration.groovy
Show resolved
Hide resolved
| @@ -16,15 +16,11 @@ class DeclarativePipeline extends GenericPipelineDeclaration { | |||
| properties.put('scm', 'scm') | |||
| } | |||
|
|
|||
| def getParams() { | |||
There was a problem hiding this comment.
If this is removed, what will happen to pipelines which call params.XYZ? Will that break?
There was a problem hiding this comment.
params is a property/variable available on binding.
Params_Jenkinsfile still works.
Depending on if params is available and XYZ in new situation i can imagine it throwing a PropertyMissingException, but im not sure returing null would help the pipeline there
src/main/groovy/com/lesfurets/jenkins/unit/declarative/NotDeclaration.groovy
Show resolved
Hide resolved
|
As far as i can see the repo is public so you should be able to checkout. All the tests that were there still work and i updated Agent_bindings_Jenkinsfile to include a method in the script which now works. Only thing that does not work is accessing the AGENT var from within the agentName() method, but this might be a groovy scoping thing. <- It is. https://code-maven.com/groovy-variable-scope -> Declare local variable with def Def creates a local variable not accessible within the agentName function. It works if i remove the def to make AGENT variable global. Opening the jenkinsFile with groovy editor also shows the compile time problem with the local var reference. So i don't see any issues left so far with missing methods or params. |
There was a problem hiding this comment.
So this PR initially looks ok to me, but admittedly it touches some things here I have no experience with (such as declarative pipelines). That said, the tests still pass, and we certainly have pipelines which use env.XYZ and params.XYZ syntax, so it's probably not harmful.
I would really appreciate if another admin could have a look before merging, though. Perhaps @stchar? 🙏
|
Hi @Willem1987, @nre-ableton, |
|
Merged. Thank you for all your efforts! |
|
Is there any upcoming release? I think with the pull requests merged i should be able to use the new version in our project. |
|
I've just releases new version, usually it takes 1-2days to replicate artifacts to public maven repository |
|
you can always checkout the tag (v1.8) and run |
Trying to leverage the default groovy delegating system to find properties and methods.
Trying to fix #276