Skip to content

[FIX JENKINS-50288] Add new option "cancelProcessOnExternalsFail" for ModuleLocation#204

Merged
kuisathaverat merged 2 commits into
jenkinsci:masterfrom
Liyaa:master
Mar 20, 2018
Merged

[FIX JENKINS-50288] Add new option "cancelProcessOnExternalsFail" for ModuleLocation#204
kuisathaverat merged 2 commits into
jenkinsci:masterfrom
Liyaa:master

Conversation

@Liyaa
Copy link
Copy Markdown
Contributor

@Liyaa Liyaa commented Jan 18, 2018

which determines if the process should be cancelled when checkout/update svn:externals failed.

Sometimes, when svn:externals checkout/update failed, the artifacts generated is likely to fall short of expectations, but now it's ignored by default and no message can found in the log.

…ch determines if the process should be cancelled when checkout/update svn:externals failed.

Sometimes, when svn:externals checkout/update failed, the artifacts generated is likely to fall short of expectations, but now it's ignored by default and no message can found in the log.
Copy link
Copy Markdown
Member

@oleg-nenashev oleg-nenashev left a comment

Choose a reason for hiding this comment

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

The code LGTM though there are some formatting changes

@@ -0,0 +1,3 @@
<div>
Determines if the process should be cancelled when checkout/update svn:externals failed. Will work when "Ignore externals" box is not checked.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Maybe it needs to be clarified to explicitly say that the default behavior is to fail the checkout

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Sorry for ignoring the code formatting while my IDE using different style, already correct it, and default behavior is clarified too.

@oleg-nenashev
Copy link
Copy Markdown
Member

CC @kuisathaverat

Copy link
Copy Markdown
Contributor

@kuisathaverat kuisathaverat left a comment

Choose a reason for hiding this comment

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

LGTM

@Liyaa
Copy link
Copy Markdown
Contributor Author

Liyaa commented Mar 6, 2018

@oleg-nenashev @kuisathaverat Thank you for your approval! Is there any other work I need to supplement? Can these commits be merged for now? ^_^

@oleg-nenashev
Copy link
Copy Markdown
Member

I think it just needs some time before we integrate everything & cut the release. ETA: this week

@kuisathaverat kuisathaverat merged commit 2959a4a into jenkinsci:master Mar 20, 2018
@kuisathaverat kuisathaverat changed the title Add new option "cancelProcessOnExternalsFail" for ModuleLocation [FIX JENKINS-50288] Add new option "cancelProcessOnExternalsFail" for ModuleLocation Mar 20, 2018
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.

3 participants