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-41272] Sketch of an APIStep #71

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jglick
Copy link
Member

@jglick jglick commented Sep 21, 2018

JENKINS-41272

Sketch of how we can provide a general step which offers the exact same information available from the “REST API” (i.e., Stapler exporting) at least for builds. Together with a simple step to set the current build’s result / description / displayName / keepLog (which would be better for Declarative anyway!), and a way for the build step to return a simple integer build number, this would allow us to deprecate the currentBuild global variable and RunWrapper generally.

@dwnusbaum
Copy link
Member

CC @mikecirioli: This might be interesting to you, particularly since it looks like it would give you what you were working on in jenkinsci/workflow-support-plugin#73 while avoiding the issue with exposing those classes directly.

@jglick
Copy link
Member Author

jglick commented Sep 21, 2018

Right, and of course this solves lots of related issues more generally. For example, you could look up culprits[fullName] to decide who to send a chat message to in case of failure. More to the point, it avoids not only sandbox issues, but potential incompatibilities with out-of-process execution.

@jglick
Copy link
Member Author

jglick commented Sep 21, 2018

Test failure probably due to one of the updates I did here to make sure all the GDK methods were whitelisted. Could fix that up if there is interest in this PR.

One thing that would be very nice is a way to simultaneously specify the tree and a way to extract the deep information up to top level, without doing complicated scripting. Would only work for cases like the one showed where you ultimately only care about a single field. Ideally would let you also filter by _class, or perhaps this is something that needs to be built into Stapler. (Or we move to GraphQL…)

@mikecirioli
Copy link

Nice @jglick - this is a cleaner implementation than my initial exploration using the data API. @cyrille-leclerc and I were discussing this topic today so I'll take a look how this can be applied to the use cases he is interested in.

@jglick
Copy link
Member Author

jglick commented Sep 24, 2018

ReadWriteFileStepTest.testKnownCharsetRoundtrip failure is probably caused by some update or another. Can look into it but first I want some design feedback here since the usability of the current sketch is not great.

@abayer
Copy link
Member

abayer commented Sep 24, 2018

This seems fantastic to me - I'd probably add a syntactic sugar step called something like buildInfo for the most common use cases (i.e., currentBuild replacement) to cut down on complexity and verbosity, but that's a nitpick.

@jglick
Copy link
Member Author

jglick commented Sep 24, 2018

syntactic sugar

You mean, Declarative? :-)

@jglick
Copy link
Member Author

jglick commented Sep 24, 2018

the most common use cases (i.e., currentBuild replacement)

Well, there is no single tree parameter which replaces all functions of currentBuild. To start with, getting information about previousBuild, upstreamBuilds, etc. would require some additions to this step to let it select a different Run, as noted in a TODO comment. Other getters like fullProjectName would require it to select a different kind of model object entirely, noted in another TODO comment.

Setting those things aside and just focusing on the current Run, first of all there are some simple fields not currently exposed via REST (like startTime); and changeSets exposes a list of complex structured objects whose exact @Exported fields cannot be known statically since this will vary according to the SCM plugin used (though many such fields are defined in core).

@abayer
Copy link
Member

abayer commented Sep 24, 2018

@jglick Yeah, I brainfarted - we will want specific setBuildResult/getBuildResult Steps at a minimum, since those tend to be the most commonly used, and we still need some way of saying "Hey, here's a pointer to the actual Run for this Pipeline that you can pass to something on the Jenkins master to actually get the Run object" (we use RunWrapper fairly extensively in Declarative for that purpose, but having the Run#getExternablizableId() as a context variable or something like that would suffice for when RunWrapper/currentBuild ends up deprecated/removed/etc), but a more generic getBuildInfo may not make sense...

Though deprecating/removing RunWrapper will also require changes over in pipeline-build-step - not sure if anything else uses it.

@jglick
Copy link
Member Author

jglick commented Oct 2, 2018

deprecating/removing RunWrapper will also require changes over in pipeline-build-step

Right, mentioned in the PR description.

@steven-foster
Copy link

This would be incredibly useful for lots of things. Would this currently be too far away from completion for anyone to reasonably pick up?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants