-
-
Notifications
You must be signed in to change notification settings - Fork 9k
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
[FIX JENKINS-41457] Use BUILD_NOW_TEXT for parameterised jobs #2735
Conversation
Asking @jglick to take a look |
String pageText = j.createWebClient().getPage(p).asText(); | ||
assertThat(pageText, containsString("newschool:aaa")); | ||
|
||
Impl.oldschool = true; |
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.
use a try
-finally
block
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.
@jglick sorry but I don't know what the try-finally is for
/** | ||
* Makes sure that {@link AlternativeUiTextProvider} actually works with a parameterized Job. | ||
*/ | ||
@Test |
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.
@Issue("JENKINS-41757")
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.
Yeah... forgot it.
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.
LGTM 👍
@@ -278,7 +278,8 @@ public static final CauseAction getBuildCause(ParameterizedJob job, StaplerReque | |||
* Uses {@link #BUILD_NOW_TEXT}. | |||
*/ | |||
public final String getBuildNowText() { | |||
return isParameterized() ? Messages.ParameterizedJobMixIn_build_with_parameters() : AlternativeUiTextProvider.get(BUILD_NOW_TEXT, asJob(), Messages.ParameterizedJobMixIn_build_now()); | |||
return isParameterized() ? AlternativeUiTextProvider.get(BUILD_NOW_TEXT, asJob(), Messages.ParameterizedJobMixIn_build_with_parameters()) |
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.
Shouldn't this rather have a new variant BUILD_WITH_PARAMETERS_TEXT
? The difference IIUC is that "Build Now" indicates an immediate action, while the other does not (like when in many applications some actions have a trailing …
and others don't).
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.
Not needed, BUILD_NOW_TEXT
is the AlternativeUiTextProvider
inside it you can check if the job has parameters and decide what text you want to display.
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.
Have a thumbs up then: 👍
@reviewbybees done |
https://issues.jenkins-ci.org/browse/JENKINS-41757
@reviewbybees @jenkinsci/code-reviewers