Allow null message in SlackMessage #31288

albendz · 2018-06-13T03:00:37Z

Adding constructors to the slack message class to support more flexibility in null parameters. Additionally provided setters for use-cases where this can be added before message template is created.

#30071

Read and executed all template tasks for contribution. Signed CLA but maybe submitted too soon for check to pass.

elasticmachine · 2018-06-13T14:04:25Z

Pinging @elastic/es-core-infra

hub-cap · 2018-06-13T14:14:07Z

@elasticmachine test this please

hub-cap · 2018-06-13T14:14:30Z

@albendz ty for the contribution. ill be pulling this down today to check it out.

hub-cap · 2018-06-13T15:28:53Z

A few comments so far. Your branch is a bit old, and this can very easily cause our CI to fail. We move fast :) We typically will push the merge commit to your remote, but I noticed that you were also requesting a PR from your master branch, and not some other branch that would be named anything you wish. I don't like to push to other people's master branch, just because I wouldn't want someone doing it to mine! It is too late for changing the branch and keeping this PR open. I think we should leave it as is, and next time you push, go ahead and do it from a aptly named branch.

You can easily merge into your master by fetching the elastic repo's changes, and just issuing a git merge upstream/master, where upstream is whatever you have named the elastic organization. I like upstream, but lesser things than naming remotes have provoked pitchforks :P This should cause CI to become happy again, and ill gladly request another round of CI testing once you update.

hub-cap

I will be testing these against our real slack install as well, so we I can verify what happens when both attachments and text are null. Again, TYVM for the contribution!!!

hub-cap · 2018-06-13T15:30:14Z

...r/src/main/java/org/elasticsearch/xpack/watcher/notification/slack/message/SlackMessage.java

-    final String text;
-    final Attachment[] attachments;
+    String text;
+    Attachment[] attachments;


What is the reason for making these non final? Our objects are typically final. And assigning null to these still counts as making sure they are final. It is very atypical for us to assign setters to objects like this to set these values after construction of the object.

I can correct this. There's no reason for them not to be final.

hub-cap · 2018-06-13T15:30:42Z

...r/src/main/java/org/elasticsearch/xpack/watcher/notification/slack/message/SlackMessage.java

+    public void setText(String text) { this.text = text;}
+
+    public void setAttachments(Attachment[] attachments) { this.attachments = attachments;}
+


see previous comment about final.

hub-cap · 2018-06-13T15:49:14Z

.../test/java/org/elasticsearch/xpack/watcher/notification/slack/message/SlackMessageTests.java

@@ -602,6 +602,11 @@ public void testUrlPathIsFiltered() throws Exception {
        }
    }

+    public void canHaveNullTextAndAttachment()  throws Exception {
+        SlackMessage slackMessage = new SlackMessage("from", new String[] {"to"}, "icon");
+        assertThat(slackMessage.getText(), is(null));


This is causing a compile error for me, pls use assertNull(object) so it compiles clean

hub-cap · 2018-06-13T15:50:27Z

One other thing I realized is that the issue itself is saying "if we have an attachment, do not require the text from the message itself". This would be better if the constructor could not pass in both attachments and text as null, but one that lets text be null if you pass an attachment in.

edit

I did a test where I removed both text and attachment, and got the following back:

"body" : "missing_text_or_fallback_or_attachments"

So this definitely wont work with both being null.

albendz · 2018-06-13T23:56:32Z

@hub-cap I'll change this so there are finals again, will make sure it's attachments OR text and not null for both. I'll update my fork as well.

albendz · 2018-06-22T19:42:57Z

@hub-cap Sorry about being so slow on this - It takes me a very long time to run the build and test. Is there a way to speed this up without losing test coverage? Are there particular hardware requirements that might make it faster to run this project?

albendz · 2018-06-22T23:22:03Z

I have created a new branch here (https://github.com/albendz/elasticsearch/tree/slack_message_empty_text). Would you like me to create a new pull request?

spinscale · 2018-06-25T08:34:47Z

.../test/java/org/elasticsearch/xpack/watcher/notification/slack/message/SlackMessageTests.java

@@ -602,6 +602,11 @@ public void testUrlPathIsFiltered() throws Exception {
        }
    }

+    public void canHaveNullTextAndAttachment()  throws Exception {


this will not get executed as a test if the method does not begin with test

hub-cap · 2018-06-25T14:07:21Z

if you prefer a new pull request, that is ok. Just be sure to reference this pull request when you open it (and close this one too!). Also, be sure to take @spinscale comment to make sure your test is run.

albendz · 2018-06-27T03:47:05Z

New Pull Request: #31596

hub-cap · 2018-06-27T14:52:59Z

There is no reason to be sorry @albendz! This stuff takes a long time :) Hours for the full check build to be run!! Sometimes it makes sense to run only the pieces that you are testing, but that requires some gradle knowledge about what you are testing, and definitely running it on newer hardware is best. Most of the engineers have newer macbook pro's or linux machines, and many of the elastic engineers run full checks on a desktop or cloud machine because it can destroy your entire laptop productivity!

I personally have a bunch of second hand dual proc server grade hardware that I run the checks on, with boatloads of ram :)

Slack accepts an empty text or attachments, but not both. This commit ensures that both are not empty when creating a watch. Closes #30071 Replacing old pull request: #31288

albendz added 2 commits June 12, 2018 19:49

Make text message not required in constructor for slack

9c8e69c

Remove unnecessary comments in test file

b6876b4

jasontedor requested a review from hub-cap June 13, 2018 03:07

colings86 added >enhancement :Data Management/Watcher labels Jun 13, 2018

hub-cap added v7.0.0 v6.4.0 labels Jun 13, 2018

hub-cap suggested changes Jun 13, 2018

View reviewed changes

hub-cap reviewed Jun 13, 2018

View reviewed changes

spinscale reviewed Jun 25, 2018

View reviewed changes

albendz mentioned this pull request Jun 27, 2018

Slack message empty text #31596

Merged

albendz closed this Jun 27, 2018

lcawl added the >non-issue label Aug 22, 2018

colings86 added v7.0.0-beta1 and removed v7.0.0 labels Feb 7, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow null message in SlackMessage #31288

Allow null message in SlackMessage #31288

albendz commented Jun 13, 2018 •

edited

Loading

elasticmachine commented Jun 13, 2018

hub-cap commented Jun 13, 2018

hub-cap commented Jun 13, 2018

hub-cap commented Jun 13, 2018

hub-cap left a comment

hub-cap Jun 13, 2018

albendz Jun 13, 2018

hub-cap Jun 13, 2018

albendz Jun 13, 2018

hub-cap Jun 13, 2018

albendz Jun 13, 2018

hub-cap commented Jun 13, 2018 •

edited

Loading

albendz commented Jun 13, 2018

albendz commented Jun 22, 2018

albendz commented Jun 22, 2018

spinscale Jun 25, 2018

hub-cap commented Jun 25, 2018

albendz commented Jun 27, 2018

hub-cap commented Jun 27, 2018

		public void setText(String text) { this.text = text;}

		public void setAttachments(Attachment[] attachments) { this.attachments = attachments;}

Allow null message in SlackMessage #31288

Allow null message in SlackMessage #31288

Conversation

albendz commented Jun 13, 2018 • edited Loading

elasticmachine commented Jun 13, 2018

hub-cap commented Jun 13, 2018

hub-cap commented Jun 13, 2018

hub-cap commented Jun 13, 2018

hub-cap left a comment

Choose a reason for hiding this comment

hub-cap Jun 13, 2018

Choose a reason for hiding this comment

albendz Jun 13, 2018

Choose a reason for hiding this comment

hub-cap Jun 13, 2018

Choose a reason for hiding this comment

albendz Jun 13, 2018

Choose a reason for hiding this comment

hub-cap Jun 13, 2018

Choose a reason for hiding this comment

albendz Jun 13, 2018

Choose a reason for hiding this comment

hub-cap commented Jun 13, 2018 • edited Loading

edit

albendz commented Jun 13, 2018

albendz commented Jun 22, 2018

albendz commented Jun 22, 2018

spinscale Jun 25, 2018

Choose a reason for hiding this comment

hub-cap commented Jun 25, 2018

albendz commented Jun 27, 2018

hub-cap commented Jun 27, 2018

albendz commented Jun 13, 2018 •

edited

Loading

hub-cap commented Jun 13, 2018 •

edited

Loading