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

Watching plus Unicode #65

Closed
wants to merge 10 commits into from

Conversation

@jglick
Copy link
Member

commented Feb 13, 2018

Integrates #63 & #64. Downstream of jenkinsci/durable-task-plugin#62.

@reviewbybees

jglick added 2 commits Feb 9, 2018
Copy link
Member

left a comment

Minor comments so far. Need to finalize the upstream PT's review before approval

@@ -102,17 +108,15 @@ public boolean isReturnStatus() {

public abstract static class DurableTaskStepDescriptor extends StepDescriptor {

public static final String defaultEncoding = "UTF-8";

public FormValidation doCheckEncoding(@QueryParameter boolean returnStdout, @QueryParameter String encoding) {

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev Mar 14, 2018

Member

Restrict it?

This comment has been minimized.

Copy link
@jglick

jglick Apr 3, 2018

Author Member

Not part of this PR, but sure.

This comment has been minimized.

Copy link
@jglick

jglick Jun 9, 2018

Author Member

(for #64)

public FormValidation doCheckEncoding(@QueryParameter boolean returnStdout, @QueryParameter String encoding) {
if (encoding.isEmpty()) {

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev Mar 14, 2018

Member

Check for null just in case the form is messed up. Ideally it makes sense to use StringUtils.isBlank() here and in the constructor

This comment has been minimized.

Copy link
@jglick

jglick Apr 3, 2018

Author Member

If the form is messed up, there is a product bug and validation will fail. No need to be unreasonably defensive.

This comment has been minimized.

Copy link
@jglick

jglick Jun 9, 2018

Author Member

(for #64)

} catch (UnsupportedOperationException x) {
getContext().onFailure(x);
} catch (Exception x) { // as below
LOGGER.log(Level.FINE, node + " is evidently offline now", x);

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev Mar 14, 2018

Member

Apparently it's not documented in jenkinsci/durable-task-plugin#60

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev Mar 14, 2018

Member

Execution link in the log would be helpful

This comment has been minimized.

Copy link
@jglick

jglick Jun 9, 2018

Author Member

(for #63)

@@ -203,6 +237,7 @@ public FormValidation doCheckReturnStatus(@QueryParameter boolean returnStdout,
if (!directory) {
throw new AbortException("missing workspace " + remote + " on " + node);
}
LOGGER.log(Level.FINE, "{0} seems to be online", node);

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev Mar 14, 2018

Member

DEBUG/FINEST? maybe also makes sense to print Execution#toString() to the log so we understand from where it comes

This comment has been minimized.

Copy link
@jglick

jglick Apr 3, 2018

Author Member

Sure.

This comment has been minimized.

Copy link
@jglick

jglick Jun 9, 2018

Author Member

(#63)

@@ -85,7 +91,7 @@ public String getEncoding() {
}

@DataBoundSetter public void setEncoding(String encoding) {
this.encoding = encoding;
this.encoding = Util.fixEmpty(encoding);

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev Mar 14, 2018

Member

Does not handle blank strings 🐜

This comment has been minimized.

Copy link
@jglick

jglick Apr 3, 2018

Author Member

Sorry, what?

Convert empty string to null.

That is exactly what I want here.

This comment has been minimized.

Copy link
@jglick

jglick Jun 9, 2018

Author Member

(anyway for #64)

Jenkinsfile Outdated
@@ -1 +1 @@
buildPlugin(jenkinsVersions: [null, '2.73.1'])

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev Mar 14, 2018

Member

Test with 2.107 instead?

This comment has been minimized.

Copy link
@jglick

jglick Jun 9, 2018

Author Member

(obsolete)

@oleg-nenashev oleg-nenashev self-requested a review Mar 14, 2018
Copy link
Member

left a comment

Requested changes in #63, #64 is approved.
Comments here have not been addressed yet. I will request changes here to remove it from the queue

jglick added 4 commits Jun 8, 2018
@jglick

This comment has been minimized.

Copy link
Member Author

commented Jun 9, 2018

Please do not perform line-by-line reviews in this PR. Rather, review component PRs.

jglick added 3 commits Jun 9, 2018
@jglick

This comment has been minimized.

Copy link
Member Author

commented Aug 7, 2018

Now covered by #63.

@jglick jglick closed this Aug 7, 2018
@jglick jglick deleted the jglick:watch-plus-UTF-8-JENKINS-31096 branch Aug 7, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.