Add ability to use comma-separated list of transitions. This allows c… #13
Conversation
…ycling through multiple transitions in a Jira workflow. If a transition is not valid for a step, it is skipped.
There was a problem hiding this comment.
Thanks for your submission @milowg! Work like yours helps build a better CI future and I appreciate you taking the time to open a pull request.
I've left some feedback requesting changes to your submission: can you first help me understand your use-case better? I've left some questions at the top of Transition class documentation.
Without my understanding your use-case just yet - I believe this is enough to warrant a new class. Your commit message contains great intent / context which is worth preserving in documentation and description of a new class. This is a departure of behavior what transition does today and its own seat at the table.
Finally, new unit tests are required before I can submit, in order to make sure nobody breaks your changes later.
| { | ||
| for (JiraCommit jiraCommit : JiraCommit.filterDuplicateIssues(jiraCommitList)) | ||
| { | ||
| AbstractBuild build, Launcher launcher, BuildListener listener) { |
There was a problem hiding this comment.
There isn't a style guide but try to be consistent with existing style - IDE might be doing this on your behalf? Brace should be on newline
| import java.util.List; | ||
| import java.util.stream.Collectors; | ||
|
|
||
| /** | ||
| * Transition a JIRA issue |
There was a problem hiding this comment.
This is a big enough change to warrant a new class. The previous behavior was determinate and easily communicated: try to call a transition on a ticket. If there's errors or it wasn't possible, log it and continue.
This new behavior is to try a transition on a ticket, and if that fails, call the next one until one works. Then go back and try all transitions.
What is the use-case here? Is it to get all tickets to a similar state (i.e closed)? IMO that is best solved with an update to Jira issue schema to allow such that as a single transition. Is there a reason that does not work, or is this for a different use-case entirely?
|
|
||
| if (!didAnyTransition) { | ||
| //No transitions were done. Show error message | ||
| listener.getLogger().println("ERROR Updating JIRA with transitions [" + transitionName + "], continuing"); |
There was a problem hiding this comment.
This does not log information on what the type of error was -- none of the provided transitions were valid for the given ticket.
Why is 'no transitions valid' considered an error condition while 'this transition is not valid' is not an error condition?
| didAnyTransition = true; | ||
| didTransition = true; | ||
| } catch (Throwable t) { | ||
| //Ignore and try next transition |
There was a problem hiding this comment.
It is probably worth logging that this transition was tried but there was some exception, which we are assuming to mean that the transition itself was not valid vs some other type of error. This at least makes visible (and thus usable) on what actions are happening -- the only way to see this currently is looking at source code.
Additionally - we need the ability to see this exception under trace / debug logging to facilitate debugging. It's possible that we are hitting some other error - we do not discriminate here, everything is caught
… when every transition fails, added unit test
|
@dalvizu I've made some changes from your PR comments. The purpose of this change is to allow Jiras to get to an end status regardless of what status it is currently in. Unfortunately, it is not always possible to change the Jira workflow to allow all paths to the end status, nor is this necessarily desirable. In my particular case, the Jira workflow is set up so developers can know which statuses should be next based on the Jira's current status. If we supported getting to each status from each other one, it would involve adding too many transitions. However, when the CI build runs, it does need to make sure that the Jira ends up in the proper status since it is done automatically without manual oversight. I don't believe that this warrants a different setting in the plugin. Users can either add comma separated transition values, or just leave one. If that is the case, then creating a separate class to handle multiple vs. single transitions may be too much. Let me know what you think. |
No description provided.