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

[WIP] Workflow build step #97

Closed
wants to merge 5 commits into from

Conversation

@tfennelly
Copy link
Member

commented Aug 28, 2014

Changes to allow the plugin be used in a Workflow. Would be good if @slide (et al) and @jglick could review. In particular... look for TODO markers in the changes I made. These were things I was not sure about.

Atm it works using the base 'step' step (i.e. you need to specify the $class) e.g.

node('master') {
    catchError {
        // Do stuff ....
    }
    step($class: 'hudson.plugins.emailext.WorkflowPublisher', recipientList: 'tfennelly@localhost', subject: 'Build failed', content: 'The build failed', attachBuildlog: true)
}

I pulled the workflow specific code up into an extension of ExtendedEmailPublisher. I think it's cleaner this way.

Once the workflow apis are published (?) we can build a dedicated step for this, it would look something like the following (see BuildStep-Job-with-step)

node('master') {
    catchError {
        // Do stuff ....
    }
    emailext(recipientList: 'tfennelly@localhost', subject: 'Build failed', content: 'The build failed', attachBuildlog: true)
}
@tfennelly

This comment has been minimized.

Copy link
Member Author

commented Aug 28, 2014

Btw... not sure I like catchError. I think something like the following would be more natural (if possible):

node('master') {
    try {
        // Do stuff ....
    } catch (Throwable t) {
        fail(t)
        emailext(recipientList: 'tfennelly@localhost', subject: 'Build failed', content: 'The build failed', attachBuildlog: true)
    }
}
@cloudbees-pull-request-builder

This comment has been minimized.

Copy link

commented Aug 28, 2014

plugins » email-ext-plugin #214 UNSTABLE
Looks like there's a problem with this pull request

@buildhive

This comment has been minimized.

Copy link

commented Aug 28, 2014

Jenkins » email-ext-plugin #6 UNSTABLE
Looks like there's a problem with this pull request
(what's this?)

Not strange that it failed on CI - strange it did not fail locally :)
@cloudbees-pull-request-builder

This comment has been minimized.

Copy link

commented Aug 28, 2014

plugins » email-ext-plugin #215 SUCCESS
This pull request looks good

@buildhive

This comment has been minimized.

Copy link

commented Aug 28, 2014

Jenkins » email-ext-plugin #7 SUCCESS
This pull request looks good
(what's this?)

@slide

This comment has been minimized.

Copy link
Member

commented Aug 28, 2014

The biggest issue I have with this pull request is bumping the core required version to something that is not an LTS. If that is required for this to work, then we'll have to wait until >= 1.577 is an LTS release. I have some other comments as well, but I'll put those directly in as line comments.

newText = TokenMacro.expandAll(context.getBuild(), context.getListener(), newText, false, macros);

// TODO: What if it's not an AbstractBuild? TokenMacro needs it.
Run<?, ?> run = context.getBuild();

This comment has been minimized.

Copy link
@slide

slide Aug 28, 2014

Member

If its not an AbstractBuild, then none of the macros will get replaced. This is a big issue IMHO, it's one of the biggest features of email-ext.

This comment has been minimized.

Copy link
@jglick

jglick Aug 28, 2014

Member

So why does TokenMacro not work with a general Run? A good reason, or just because no one ever cared before?

This comment has been minimized.

Copy link
@tfennelly

tfennelly Aug 31, 2014

Author Member

Apart from just looking into the classes and methods that this plugin interacts with in TokenMacro, I haven't really looked at it in detail. One thing I do remember about it was that in some parts of it it tried to get access to build specific entities such as participants and culprits, which I think were in AbstractBuild but not in Run. They sound like things that could be in Run though, yes/no?

I'll get the source for it and see what would be involved in generalising it to use Run.

This comment has been minimized.

Copy link
@jglick

jglick Sep 1, 2014

Member

Support for “culprits” and the like is not available in Run yet. I am not sure what Token Macro needs that information for, but for now it could just check for instanceof AbstractBuild, while still having unrelated things work on any Run.

@jenkinsadmin

This comment has been minimized.

Copy link
Member

commented Aug 28, 2014

Thank you for a pull request! Please check this document for how the Jenkins project handles pull requests

@@ -85,7 +85,8 @@ public String evaluate(AbstractBuild<?, ?> build, TaskListener listener, String
buf.append(def);
}
if (showDependencies) {
AbstractBuild previousBuild = ExtendedEmailPublisher.getPreviousBuild(build, listener);
// TODO: Must be an AbstractBuild?
AbstractBuild previousBuild = (AbstractBuild) ExtendedEmailPublisher.getPreviousBuild(build, listener);

This comment has been minimized.

Copy link
@jglick

jglick Aug 28, 2014

Member

Run does not yet have any ways of getting dependencies. This would require a broader refactoring in core. Which could be done, though I expect many workflows to implement a complete pipeline in one Job (this is one of the advantages), so they would not need such a feature. There are less common cases where you would care (such as import of artifacts produced by a job maintained by a different team).

@jglick

This comment has been minimized.

Copy link
Member

commented Aug 28, 2014

Why do you need to introduce WorkflowPublisher at all? This seems to defeat the purpose of using the new SimpleBuildStep API, which is to allow the existing ExtendedEmailPublisher to be used directly without any custom logic. (See jenkinsci/mailer-plugin#15 for comparison.)

The catchError step exists specifically so that standard notifiers can be used even if their behavior depends on the current build’s expected exit status (as this one does). If you have a dedicated Step then you would just use a plain try block. (Such a Step could in principle be added to plugins today, since workflow-step-api is tiny and has no dependencies; though since the API may not yet be stable we prefer to keep the implementations inside workflow-plugin for now.)

@tfennelly

This comment has been minimized.

Copy link
Member Author

commented Aug 31, 2014

I would have thought the main reason for SimpleBuildStep was to allow plugins play in Workflows without depending on the workflow APIs. Either way, I don't really see how it defeats the purpose of using it just by using it in a slightly different way (extend and implement Vs directly implementing).

I actually did have it implemented directly on ExtendedEmailPublisher but I just thought that it was cleaner to pull it out into a class of it's own. That's purely subjective though of course. Also, I didn't want to go renaming the args on the DataBoundConstructor on ExtendedEmailPublisher (was not sure what the possible side effects of that might be wrt to Jenkins reflection magic), while at the same time I didn't want users having to specify workflow params with names like project_default_subject or with names and values like project_attach_buildlog: 1, project_attach_buildlog: 2 etc.

All that said... it's no big deal to pull it back into ExtendedEmailPublisher :)

@slide

This comment has been minimized.

Copy link
Member

commented Aug 31, 2014

Could it be in a separate wrapper plugin instead?
On Aug 31, 2014 2:31 AM, "Tom Fennelly" notifications@github.com wrote:

I would have thought the main reason for SimpleBuildStep was to allow
plugins play in Workflows without depending on the workflow APIs. Either
way, I don't really see how it defeats the purpose of using it just be
using it in a slightly different way (extend and implement Vs directly
implementing).

I actually did have it implemented directly on ExtendedEmailPublisher but
I just thought that it was cleaner to pull it out into a class of it's own.
That's purely subjective though of course. I didn't want to go renaming the
args on the DataBoundConstructor on ExtendedEmailPublisher (was not sure
what the possible side effects of that might be wrt to Jenkins reflection
magic), while at the same time I didn't want users having to specify
workflow params with names like project_default_subject or with names and
values like project_attach_buildlog: 1, project_attach_buildlog: 2 etc.

All that said... it's no big deal to pull it back into
ExtendedEmailPublisher :)


Reply to this email directly or view it on GitHub
#97 (comment)
.

@jglick

This comment has been minimized.

Copy link
Member

commented Sep 1, 2014

I just thought that it was cleaner to pull it out into a class of it's own.

The idea of SimpleBuildStep is to have one build step class which can be used in different ways.

names like project_default_subject

That ought to be renamed IMO to match the actual field name defaultSubject. This would allow the config.groovy to be simplified from

f.entry(title: _("Default Subject"), help: "/plugin/email-ext/help/projectConfig/defaultSubject.html") {
  f.textbox(name: "project_default_subject", value: configured ? instance.defaultSubject : "\$DEFAULT_SUBJECT") 
}

to the more standard

f.entry(field: 'defaultSubject', title: _("Default Subject")) {
  f.textbox('default': "\$DEFAULT_SUBJECT")
}

assuming src/main/webapp/help/projectConfig/defaultSubject.html were also moved to the standard location src/main/resources/hudson/plugins/emailext/ExtendedEmailPublisher/help-defaultSubject.html.

values like project_attach_buildlog: 1

Trickier since AFAIK form binding does not let you conditionally show a checkbox. Better would be to use an enum capturing the three possible states with sensible names, in which case

f.entry(field: 'buildLog', title: _("Attach Build Log")) {
  f:select
}

would suffice. Requires some minor readResolve work to convert stored attachBuildLog and compressBuildLog fields.

Also this would be the right time to deprecate the existing constructors, introducing one new @DataBoundConstructor with only mandatory fields (just recipientList, or maybe even that is optional sometimes?), and using @DataBoundSetter for everything else. This documentation for SCMs is applicable also to build steps.

@slide

This comment has been minimized.

Copy link
Member

commented Sep 1, 2014

There were people in other plugins who are using the names from the fields
as is, so I can't really change them.

On Mon, Sep 1, 2014 at 2:56 PM, Jesse Glick notifications@github.com
wrote:

I just thought that it was cleaner to pull it out into a class of it's own.

The idea of SimpleBuildStep is to have one build step class which can be
used in different ways.

names like project_default_subject

That ought to be renamed IMO to match the actual field name defaultSubject.
This would allow the config.groovy to be simplified from

f.entry(title: _("Default Subject"), help: "/plugin/email-ext/help/projectConfig/defaultSubject.html") {
f.textbox(name: "project_default_subject", value: configured ? instance.defaultSubject : "$DEFAULT_SUBJECT")
}

to the more standard

f.entry(field: 'defaultSubject', title: _("Default Subject")) {
f.textbox('default': "$DEFAULT_SUBJECT")
}

assuming src/main/webapp/help/projectConfig/defaultSubject.html were also
moved to the standard location
src/main/resources/hudson/plugins/emailext/ExtendedEmailPublisher/help-defaultSubject.html
.

values like project_attach_buildlog: 1

Trickier since AFAIK form binding does not let you conditionally show a
checkbox. Better would be to use an enum capturing the three possible
states with sensible names, in which case

f.entry(field: 'buildLog', title: _("Attach Build Log")) {
f:select
}

would suffice. Requires some minor readResolve work to convert stored
attachBuildLog and compressBuildLog fields.

Also this would be the right time to deprecate the existing constructors,
introducing one new @DataBoundConstructor with only mandatory fields
(just recipientList, or maybe even that is optional sometimes?), and
using @DataBoundSetter for everything else. This documentation
https://github.com/jenkinsci/workflow-plugin/blob/master/scm-step/README.md#constructor-vs-setters
for SCMs is applicable also to build steps.


Reply to this email directly or view it on GitHub
#97 (comment)
.

Website: http://earl-of-code.com

@jglick

This comment has been minimized.

Copy link
Member

commented Sep 2, 2014

There is a little trick for converting fields to methods (at which point you have more flexibility about storage):

@Deprecated @AdaptField public boolean attachBuildLog() {…}
@Deprecated public void attachBuildLog(boolean newValue) {…}
@tfennelly

This comment has been minimized.

Copy link
Member Author

commented Sep 2, 2014

@slide In some way I think a partner/shim plugin that adds workflow could make sense. However, I think that would go against the original idea of SimpleBuildStep even more than a separate class.

@jglick Okay, thanks Jesse. I'll redo it the way you prefer. Hopefully @slide will be okay with that too. Tbh though... the points you made above only proved to convince me even more that a separate class is cleaner :)

@slide

This comment has been minimized.

Copy link
Member

commented Sep 2, 2014

@jglick That works with a @DataBoundConstructor?

@jglick

This comment has been minimized.

Copy link
Member

commented Sep 2, 2014

@tfennelly if you prefer to do an explicit Step then for the near term it can just live in a module of workflow-plugin. The idea with SimpleBuildStep is that you do some cleanup of the general build step which is useful anyway (makes it easier to add/modify parameters, simplifies form binding, cleans up code), and by implementing a simple interface you make the build step more broadly usable. But if it is too much hassle then close this PR and skip it. I do not have a strong opinion about it.

@slide @AdaptField is for fixing the historical mistake of making your instance fields public. You can then introduce a better private field, use readResolve to upgrade storage, and have whatever @Deprecated accessor methods are needed to maintain binary compatibility. This has little to do with the constructor: for that, just introduce a new @DataBoundConstructor with few (or no) arguments matched by @DataBoundSetters, and simply mark any existing constructors @Deprecated (keeping their previous behavior).

@tfennelly

This comment has been minimized.

Copy link
Member Author

commented Sep 2, 2014

@jglick It's not too much hassle at all. I'm currently looking at what would be involved in refactoring TokenMacro. I guess it all depends on that i.e. if refactoring TokenMacro (to use RUN Vs AbstractBuild) is not doable then I guess this PR is stuck. If it can be refactored then I'll fix up this PR and put WorkflowPublisher back into ExtendedEmailPublisher.

@tfennelly

This comment has been minimized.

Copy link
Member Author

commented Sep 2, 2014

@jglick @slide I made refactorings to Jenkins core and token-macro-plugin to get an idea of what would be needed to make the token-macro-plugin work with Run Vs AbstractBuild. It looks doable but I suppose the main question is are the changes I had to make to AbstractBuild and Run okay or are they going to screw other things up?

There's also a question on the changes I made to token macro wrt anything that depends on it and extends the TokenMacro class. The changes I made would mean these extensions would need to change in order for them to compile against the new token-macro. If that's an issue, the only way I can currently think of getting around it is to create a new TokenMacro impl deprecate the old one.

@slide

This comment has been minimized.

Copy link
Member

commented Sep 2, 2014

Its probably a question for a wider audience as token macro is used by a
bunch of plugins.
On Sep 2, 2014 11:36 AM, "Tom Fennelly" notifications@github.com wrote:

@jglick https://github.com/jglick @slide https://github.com/slide I
made refactorings to Jenkins core
https://github.com/tfennelly/jenkins/compare/token-macro-to-run and
token-macro-plugin
https://github.com/tfennelly/token-macro-plugin/compare/token-macro-to-run
to get an idea of what would be needed to make the token-macro-plugin work
with Run Vs AbstractBuild. It looks doable but I suppose the main
question is are the changes I had to make to AbstractBuild and Run okay
or are they going to screw other things up?

There's also a question on the changes I made to token macro wrt anything
that depends on it and extends the TokenMacro class. The changes I made
would mean these extensions would need to change in order for them to
compile against the new token-macro. If that's an issue, the only way I can
currently think of getting around it is to create a new TokenMacro impl
deprecate the old one.


Reply to this email directly or view it on GitHub
#97 (comment)
.

@tfennelly

This comment has been minimized.

Copy link
Member Author

commented Sep 2, 2014

@slide That's fair enough... I was thinking we'd need to do that but I wanted to run it by you guys first for a sanity check. If ye think it's okay, then I'll create PRs etc for token-macro.

- Merged WorflowPublisher back into ExtendedEmailPublisher
- Refactorings to work with refactored Core 1.582-SNAPSHOT (Run instread AbstractBuild)
- Refactorings to work with refactored TokenMacro (using core 1.582-SNAPSHOT)
@buildhive

This comment has been minimized.

Copy link

commented Sep 15, 2014

Jenkins » email-ext-plugin #9 FAILURE
Looks like there's a problem with this pull request
(what's this?)

@tfennelly

This comment has been minimized.

Copy link
Member Author

commented Sep 15, 2014

I started a dev list email thread about the core changes I made. No responses to it though.

@slide

This comment has been minimized.

Copy link
Member

commented Nov 25, 2014

@tfennelly The workflow plugin uses the constructor, or does it use the field names?

@jglick

This comment has been minimized.

Copy link
Member

commented Nov 25, 2014

@DataBoundConstructor parameter names and @DataBoundSetter feld/method names are used as the names of parameters for steps and nested Describable objects.

@slide

This comment has been minimized.

Copy link
Member

commented Nov 26, 2014

@tfennelly I did some looking into token-macro and changing it to use Run instead of AbstractBuild and I was able to maintain the methods that currently take AbstractBuild (to maintain binary compatibility) but also support Run. I'll push to a branch and you can take a look to see if it does what you think is needed.

@slide

This comment has been minimized.

Copy link
Member

commented Nov 28, 2014

@jglick I am using AdaptField and just want to make sure I am using it correctly, since I am not really a Java person.

I had:

public String recipientList = "";

I changed to

private String recipientList = "";

@AdaptField(name="recipientList", was=String.class)
public String getRecipientList() {
    return recipientList;
}

@DataBoundSetter
@AdaptField(name="recipientList", was=String.class)
public void setRecipientList(String recipientList) {
    this.recipientList = recipientList;
}

Will this work, or will just making the field private but keeping the same name cause issues?

@jglick

This comment has been minimized.

Copy link
Member

commented Dec 1, 2014

I did not write this annotation and am not completely sure how it works; its Javadoc is vague. @kohsuke would know best. This doc is all I know.

@jglick

This comment has been minimized.

Copy link
Member

commented Jan 23, 2015

Given jenkinsci/pipeline-plugin#39 I wonder if this should just be closed: it is more straightforward to just use a plain mail step with some inline Groovy templating then configure a very complex step in the Snippet Generator UI and paste it in. There would still be some reasons some form of integration with this plugin could be fruitful:

  • If some of the specialized kinds of content (mainly for the message body I think) require complex calculations that it would be unpleasant to write from scratch. For example, WorkspaceFileContent adds little value to a workflow, and ScriptContent is out of the question, but BuildLogRegexContent is doing real work that could not easily be replicated.
  • If some of the triggers are not trivially written inline in the flow. (Assuming that the current WorkflowRun and relevant methods were made accessible: discussion.) ImprovementTrigger is a good example.
  • Custom RecipientProviders, assuming some API changes in core to make it possible to use them on non-AbstractBuilds.

In other words, rather than offering ExtendedEmailPublisher as a monolithic build step, offer individual steps which wrap its useful extension points à la carte, so that a workflow could be written which (after catchError) assembles some canned functionality along with short bits of customized logic and finally calls mail with the result.

@rpavlik rpavlik referenced this pull request Mar 12, 2015
@s0undt3ch

This comment has been minimized.

Copy link

commented Mar 27, 2015

👍

Recreating the email body logic is possible, but preferably done outside of the workflow script

@slide slide closed this Apr 8, 2015
@AnalogJ

This comment has been minimized.

Copy link

commented May 7, 2015

Sorry to dig this up again. After looking at this PR and some of the other PR's in the workflow plugin I'm a bit confused. I'm trying to figure out if the email-ext plugin or the mail step in the workflow plugin supports email attachments yet.

Thanks

@slide

This comment has been minimized.

Copy link
Member

commented May 7, 2015

I've never used the workflow plugin, so I don't know how it works with email-ext. Email-ext itself supports attachments, but I don't know how that is exposed in workflow, if at all.

@jglick

This comment has been minimized.

Copy link
Member

commented May 8, 2015

@AnalogJ better to use a different forum such as Stack Overflow for questions, or just use JIRA. (In this case, no, there is no attachment support yet.)

@AnalogJ

This comment has been minimized.

Copy link

commented May 10, 2015

@jglick @slide Thanks for following up. The only reason I posted here is that the workflow-plugin compatibility page specifically links to this PR. I'll make a new question on SO for a mail attachment workaround.

Thanks!

@vbansal22

This comment has been minimized.

Copy link

commented Sep 11, 2015

Sorry to stir the pot again. Attachment support is a much wanted feature in the workflow mail step. Having just "Please go to ${env.BUILD_URL}." in the body is not sufficient because the recipient could essentially be viewing the message on a mobile phone and may not have access to the build machine at that time. Is there a way to read the build log from somewhere in the script itself?

@vbansal22

This comment has been minimized.

Copy link

commented Sep 11, 2015

Getting the build output in the email is not such a big issue. I could do it this way.

try {
    ...
}  catch (e)  {
    sh "curl ${env.BUILD_URL}/consoleText -o buildoutput"
    def buildoutput = readFile('buildoutput')
    mail (to: 'abc@example.com',
    from: 'build-bot-donot-reply@example.com',
    subject: "Job '${env.JOB_NAME}' (${env.BUILD_NUMBER}) failed ",
    body: "Please go to ${env.BUILD_URL}. or see as follows: ${buildoutput}");
    throw e
}
@jglick

This comment has been minimized.

Copy link
Member

commented Sep 14, 2015

@vbansal22 probably what you are looking for is JENKINS-28119.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
9 participants
You can’t perform that action at this time.