-
Notifications
You must be signed in to change notification settings - Fork 57
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
Change the build status to Unstable when Jira Issues not DONE and deploying to D #664
Conversation
message += '\n\n' + type.capitalize() + ': ' + values.join(', ') | ||
} | ||
} | ||
message |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you get the chance to, please use return statements even though Groovy does not require it, but aids readability.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is done in the new class afte refactoring that we have created to deal with the message as @angelmp01 requested
def cannotContinueAsHasOpenIssuesInClosingRelease = false | ||
try { | ||
result = new InitStage(this, project, repos, startAgentStage).execute() | ||
} catch (OpenIssuesException ex) { | ||
cannotContinueAsHasOpenIssuesInClosingRelease = true | ||
} | ||
if (cannotContinueAsHasOpenIssuesInClosingRelease) { | ||
logger.warn('Cannot continue as it has open issues in the release.') | ||
return | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jorge-romero why rather complex? wouldn't the logger.warn + return inside the catch block be enough?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's was my first implementation, but it was complaining about trying to finalize the pipeline within a catch block, that's why I had to rewrite in this ugly way unfortunally
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another solution may be logger.warn + result=false inside the catch block. And remove cannotContinueAsHasOpenIssuesInClosingRelease var.
message += '\n\n' + type.capitalize() + ': ' + values.join(', ') | ||
} | ||
} | ||
String message = generateWIPIssuesMessage() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you use only the message inside the if, create it inside
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you check the whole code as in the diff looks different it is inside the if block :)
@@ -406,6 +397,21 @@ class Project { | |||
return this | |||
} | |||
|
|||
def generateWIPIssuesMessage() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we can move the responsability of generate this message to another class instead Project class. Think that project class is too big.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moved to another class, and moved the tests for that specific functionallity to a new Spec for that class
Change to unstable the result of the build in case of documents not closed