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-29922] $class must die #28

Merged
merged 53 commits into from Jul 28, 2016

Conversation

Projects
None yet
4 participants
@kohsuke
Copy link
Member

kohsuke commented Jun 30, 2016

  • allow Describable to be run like a step via meta-step
  • handle nested Describable correctly
  • handle single argument construction like f("foo")
  • split arguments to Describable and metastep
  • write some test cases
  • update Snippetizer
  • add some more snippetizer tests
@kohsuke

This comment has been minimized.

Copy link
Member Author

kohsuke commented Jun 30, 2016

See JENKINS-29922 for all the other relevant PRs that span across multiple plugins.

@jglick jglick closed this Jul 11, 2016

@jglick jglick reopened this Jul 11, 2016

jglick and others added some commits Jul 11, 2016

[JENKINS-29922] make snippetizer work with symbols and meta-step
The first step that still doesn't handle use of symbols in nested objects.
[JENKINS-29922] made this work with nested objects
Reorganized the whole writing around 'x2object(StringBuilder,Object)'
form
invokeDescribable might still take a closure
because the meta-step takes one.
Handle nested expressions and brackets
It's not very clear exactly when parenthesis can be omitted, so after talking to Jesse a bit we decided to just omit that at the top level
@kohsuke

This comment has been minimized.

Copy link
Member Author

kohsuke commented Jul 12, 2016

I claim code complete

Fixing a regression
There's a difference between (1) whether symbol needs to be exploded to a literal map and (2) whether a meta-step can be used.

(2) is a subset of (1)

@kohsuke kohsuke force-pushed the JENKINS-29922 branch from 5b026fd to 04058d0 Jul 12, 2016

@kohsuke kohsuke closed this Jul 12, 2016

@kohsuke kohsuke reopened this Jul 12, 2016

@kohsuke

This comment has been minimized.

Copy link
Member Author

kohsuke commented Jul 12, 2016

Come on Jenkins, let's try this again

@jglick jglick closed this Jul 27, 2016

@jglick jglick reopened this Jul 27, 2016

}

@Issue("JENKINS-29922")
@Test public void runMetaBlockStep() throws Exception {

This comment has been minimized.

Copy link
@abayer

abayer Jul 27, 2016

Member

Would still like to see tests/how this would look for properties, but that's probably fairly special-casing and would likely be in workflow-multibranch-plugin anyway.

This comment has been minimized.

Copy link
@jglick

jglick Jul 27, 2016

Member

IIRC @kohsuke did some temporary sanity checks before reverting, but you are right it would be helpful to have these tests codified. I will play with it.

@abayer

This comment has been minimized.

Copy link
Member

abayer commented Jul 27, 2016

Only question I have is probably a pretty simple one - this won't break existing step([[$class: ... ]]) invocations, right?

@abayer

This comment has been minimized.

Copy link
Member

abayer commented Jul 27, 2016

Answered by tests over at jenkinsci/workflow-basic-steps-plugin#10 so we're good - 🐝

@jglick

This comment has been minimized.

Copy link
Member

jglick commented Jul 27, 2016

this won't break existing step([[$class: ... ]]) invocations, right?

Right, those will continue to work as before, and in fact Snippet Generator will still offer that syntax when it cannot do better.

@kohsuke

This comment has been minimized.

Copy link
Member Author

kohsuke commented Jul 27, 2016

🐝

@jglick jglick merged commit 7703fa5 into master Jul 28, 2016

1 check passed

Jenkins This pull request looks good
Details

@jglick jglick deleted the JENKINS-29922 branch Jul 28, 2016

svanoort added a commit that referenced this pull request Jun 5, 2017

Merge pull request #28 from jenkinsci/stepnode
Add StepNode to support fetching StepDescriptor at higher level of API
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.