Skip to content
This repository has been archived by the owner on Dec 15, 2021. It is now read-only.

[JENKINS-26126] Syntax schema #275

Merged
merged 9 commits into from Dec 16, 2015
Merged

Conversation

jglick
Copy link
Member

@jglick jglick commented Dec 15, 2015

JENKINS-26126

  • API design
  • basic tests
  • tests for help text
  • implementation
  • prototype static documentation generation

@reviewbybees esp. @abayer

@abayer
Copy link
Member

abayer commented Dec 15, 2015

Yaaaay - looking forward to utilizing this a bunch.

@ghost
Copy link

ghost commented Dec 15, 2015

This pull request originates from a CloudBees employee. At CloudBees, we require that all pull requests be reviewed by other CloudBees employees before we seek to have the change accepted. If you want to learn more about our process please see this explanation.

@amuniz
Copy link
Member

amuniz commented Dec 16, 2015

@jglick It seems like this PR is partially commited to master (?) 3a3d8a1

@amuniz
Copy link
Member

amuniz commented Dec 16, 2015

Oh, ok, it's not master, it's your feature branch, is there any reason to use an origin branch in this case instead of a fork?

@amuniz
Copy link
Member

amuniz commented Dec 16, 2015

BTW every commit in this PR is spamming the JIRA issue with comments.

} else if (type instanceof DescribableHelper.HeterogeneousObjectType) {
pw.println("<p><strong>Nested choice of objects</strong></p><ul>");
for (Map.Entry<String,DescribableHelper.Schema> entry : ((DescribableHelper.HeterogeneousObjectType) type).getTypes().entrySet()) {
pw.println("<li><code>$class: '" + entry.getKey() + "'</code></li>");
Copy link
Member

Choose a reason for hiding this comment

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

DescribableHelper.CLAZZ instead of $class?

@abayer
Copy link
Member

abayer commented Dec 16, 2015

Prototype site generation looks like a great start - obviously lots of tweaks to the presentation needed, and we need to add GDSL/DSLD generation, but that all can come after the guts here go in. Can't speak to the code specifically, but the output seems to do all we need it to do.

@abayer
Copy link
Member

abayer commented Dec 16, 2015

I'm 🐝 on this - it may not be perfect yet, but it's far enough that it can be built on for more.

@jglick
Copy link
Member Author

jglick commented Dec 16, 2015

is there any reason to use an origin branch in this case instead of a fork?

Yes—@abayer said he might want to work on it directly. But if he agrees with the contents so far, it could just be merged.

every commit in this PR is spamming the JIRA issue with comments.

I know, this is a bug in the JIRA link daemon, that it does not pay attention to branch. Feel free to fix it.

@abayer
Copy link
Member

abayer commented Dec 16, 2015

@abayer
Copy link
Member

abayer commented Dec 16, 2015

@jglick - curious - do you remember where the JIRA link daemon source is?

@jglick
Copy link
Member Author

jglick commented Dec 16, 2015

do you remember where the JIRA link daemon source is?

No, ask on #infra.

@KostyaSha
Copy link
Member

@abayer separate org in GH jenkins-infra

…n there happened to be no implementations loaded.
@amuniz
Copy link
Member

amuniz commented Dec 16, 2015

do you remember where the JIRA link daemon source is?

I was going to ask that :) If you point me to that repo I'll fix it.

About the HTML format, I would suggest a little change: move the Type to same line that the attr name.
For example:

archive: Archive artifacts

Archives build output artifacts for later use.

includes (Type:String)
Include artifacts matching this Ant style pattern. Use a comma separated list to add more than one expression.

I know this clean up work will be done later in sepatate PRs, but I'm sure I'll forget to send this comment later, so here it is 😄

@jglick
Copy link
Member Author

jglick commented Dec 16, 2015

About the HTML format, I would suggest a little change

Oh, there are many changes that should be made. The current output is just a proof of concept to validate that the relevant data is included.

@abayer
Copy link
Member

abayer commented Dec 16, 2015

So starting some work on gdsl generation and there is one thing we're not reporting for steps, so far as I can tell - return type. It's generally void, but there are some cases (readFile, e.g.) where it's something else, and gdsl, at least, needs that.

@jglick
Copy link
Member Author

jglick commented Dec 16, 2015

there is one thing we're not reporting for steps, so far as I can tell - return type

Sorry, this information is not available by reflection. Anyway several steps return different types depending on the options you selected. I think all we can do is display the help.html and treat the return type as Object for purposes of completion.

I somehow lucked out and got a clean build, so seeking rebees (or whatever you think appropriate) from @abayer / @amuniz. With this in master, various data sinks could be done as parallel PRs.

@abayer
Copy link
Member

abayer commented Dec 16, 2015

🐝 it is.

@abayer
Copy link
Member

abayer commented Dec 16, 2015

Ah, I see that @rsandell's script does that via Groovy magic and guesswork - https://github.com/abayer/workflow-experiments/blob/master/workflow-gdsl.groovy#L125

@abayer
Copy link
Member

abayer commented Dec 16, 2015

@jglick Is there a way to tell whether the step has a return value at all vs a void one? If I can't even tell that, either all steps are going to have to go in the GDSL as return type void or as return type Object...

@amuniz
Copy link
Member

amuniz commented Dec 16, 2015

🐝

@jglick
Copy link
Member Author

jglick commented Dec 16, 2015

via Groovy magic and guesswork

Only works for steps which happen to use AbstractSynchronousStepExecution or AbstractSynchronousNonBlockingStepExecution; will not work for asynchronous steps, or for steps which choose to use a different StepExecution implementation for whatever reason. (I think it was with @tfennelly, but maybe @amuniz, that I discussed the possibility of deprecating AbstractStepImpl, AbstractStepDescriptorImpl, AbstractStepExecutionImpl, and StepContextParameter in favor of simple nonreflective code, in which case AbstractSynchronousStepExecution and AbstractSynchronousNonBlockingStepExecution would also be deprecated and replaced. In such a case there is no way such guesses would work for any steps.)

Is there a way to tell whether the step has a return value at all vs a void one?

No. All you can do is treat them as returning Object (which subsumes a null return value ~ void).

If information about the return value is critical, then we will need a new API in StepDescriptor. Again there are some existing steps which would have to give the value as Object.class since their return type can vary according to the mode of invocation; in many cases you could pin down a specific type based on a Step instance, but that is useless for your purposes anyway.

Also note that many block-scoped steps—but not all—return any value returned by (some invocation of) their block. So the logical type is something like

<T> T theStep(String someParameter, Closure<T> body)

@abayer
Copy link
Member

abayer commented Dec 16, 2015

Ok, for now, I'll just go with Object and we'll see if that creates any problems.

@jglick jglick merged commit 43cb95a into master Dec 16, 2015
jglick added a commit that referenced this pull request Dec 16, 2015
@jglick jglick deleted the DescribableHelper-schema-JENKINS-26126 branch December 16, 2015 19:39
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
4 participants