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-54323] Add setBuildInfo step #75

Closed
wants to merge 1 commit into from

Conversation

dwnusbaum
Copy link
Member

@dwnusbaum dwnusbaum commented Oct 29, 2018

See JENKINS-54323. Complements #71.

This PR adds a dedicated step named setBuildInfo (I don't feel too strongly about that name, so let me know if you'd prefer something else) for setting Run.description, Run.displayName, Run.keepLog, and Run.result. This step would get rid of one of the use cases that currently requires RunWrapper.

Right now, I've created a single step that takes a specified Describable in its constructor:

setBuildInfo(description('description here'))
setBuildInfo(displayName('display name here'))
setBuildInfo(keepLog(true))
setBuildInfo(result('FAILURE'))

The other approach that would make sense to me would be to have a dedicated step for each field:

setBuildDescription('description here')
setBuildDisplayName('display name here')
setBuildKeepLog(true)
setBuildResult('FAILURE')

I chose the describable approach because I initially allowed a list to be passed to the step, but that didn't seem very likely to be useful in practice, so I simplified it, at which point a dedicated step for each field seems like the better approach. I'm putting this up as-is to see what people think, so let me know if you have a strong preference for either implementation.

@reviewbybees

Copy link
Member

@abayer abayer left a comment

Choose a reason for hiding this comment

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

Looks good at first glance to me - do we want to have a get equivalent as well?

@dwnusbaum
Copy link
Member Author

@abayer Yeah, I wasn't sure if we wanted that to be handled along with the API step (i.e. as shims on top of specific API queries), or if we should just implement the common ones directly in the obvious way. Thoughts?

@abayer
Copy link
Member

abayer commented Oct 29, 2018

Oh yeah, forgot about that. Well...hmm. I'd say that if we're exposing set for these things, get as well via the same approach would only make sense.

@jglick
Copy link
Member

jglick commented Nov 6, 2018

The api step already handles the get case (for both the current build and others), so I see no reason to bother with that. For compatibility, the currentBuild shim in off-master would manage gets.

@Override
public Void run() throws Exception {
Run r = getContext().get(Run.class);
if (r != null) {
Copy link
Member

Choose a reason for hiding this comment

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

It is required context; you need not and should not check for null.

Copy link
Member

Choose a reason for hiding this comment

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

IDEs tend to glare at you if you don't. =)

}
}

@Restricted(NoExternalUse.class)
Copy link
Member

Choose a reason for hiding this comment

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

Or could be made an ExtensionPoint if the need arises.


@Restricted(NoExternalUse.class)
public static abstract class RunPropertySetter extends AbstractDescribableImpl<RunPropertySetter> {
public abstract void set(Run r) throws Exception;
Copy link
Member

Choose a reason for hiding this comment

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

rawtypes

@jglick
Copy link
Member

jglick commented Nov 6, 2018

a dedicated step for each field seems like the better approach

No particular preference. I suppose the dedicated step is less verbose for users.

@dwnusbaum
Copy link
Member Author

No plans to pick this back up in the near future.

@dwnusbaum dwnusbaum closed this Apr 20, 2020
@dwnusbaum dwnusbaum deleted the JENKINS-54323 branch April 20, 2020 17:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants