-
Notifications
You must be signed in to change notification settings - Fork 30
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
Add Action to post message to slack #1242
Conversation
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.
Please see comments
horreum-backend/src/main/java/io/hyperfoil/tools/horreum/action/SlackPluginBase.java
Outdated
Show resolved
Hide resolved
|
||
@Override | ||
public void validate(JsonNode config, JsonNode secrets) { | ||
log.infof("Validating config %s, secrets %s", config, secrets); |
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.
Please can this be a TRACE 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.
I must have missed this when I was cleaning up: I'd meant to remove it, although it was really useful in initial testing to confirm that it had been called.
ObjectMapper mapper = new ObjectMapper(); | ||
ObjectNode body = mapper.createObjectNode(); |
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.
We should re-use Util.OBJECT_MAPPER
here
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.
Ah, thanks: I'd expected there must be an object mapper already instantiated somewhere, but I failed to find it.
.setHost("slack.com") | ||
.setPort(443) | ||
.setURI("/api/chat.postMessage") | ||
.setSsl(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.
These values should be configurable (I see it is not in the github example, but I think that is an oversight), with these specific values as the default. This will allow us to make changes to service config if either a) anything changes in the future b) if we want to mock a slack service in testing
Please see https://github.com/Hyperfoil/Horreum/blob/master/horreum-backend/src/main/java/io/hyperfoil/tools/horreum/action/HttpAction.java#L36 for configuration examples with a default values
We could then define difference profiles in application.properties
;
%prod.horreum.action.slack.host=slack.com
%prod.horreum.action.slack.port=443
%prod.horreum.action.slack.ssl=true
%test.horreum.action.slack.host=localhost
%test.horreum.action.slack.port=8081
%test.horreum.action.slack.ssl=false
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.
I'd initially toyed with various levels of URI configurability. For example, until I got the Slack auth token mechanism figured out I wasn't entirely sure that I could ignore Slack's subdomains for workspaces (e.g., redhat-internal.slack.com). In the end I determined there wasn't anything that needed to be user-configurable: but your suggestion of making it application configurable makes sense.
I think I'll follow the reasonable HTTP action model, accept a full URL and break it down using the URL
class rather than configuring each part separately, so it'd just be more like %test.horreum.action.slack.url=http://localhost:8081/api/slack
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.
Sounds like a good idea
horreum-backend/src/test/java/io/hyperfoil/tools/horreum/svc/ActionServiceTest.java
Show resolved
Hide resolved
// If only ... | ||
// Assert.assertTrue("No slack action errors found", errors > 0); |
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 we mock the slack service and use the mocked endpoint for testing, we can control the reponse, and therefore the errors we see in testing. A mocked service could be something as simple as ;
package io.hyperfoil.tools.horreum;
import com.fasterxml.jackson.databind.ObjectMapper;
import com.fasterxml.jackson.databind.node.ObjectNode;
import jakarta.enterprise.context.ApplicationScoped;
import jakarta.inject.Inject;
import jakarta.ws.rs.Consumes;
import jakarta.ws.rs.POST;
import jakarta.ws.rs.Path;
import jakarta.ws.rs.Produces;
import jakarta.ws.rs.core.MediaType;
import org.eclipse.microprofile.openapi.annotations.tags.Tag;
import org.jboss.logging.Logger;
@ApplicationScoped
@Path("/api/chat.postMessage")
@Consumes(MediaType.APPLICATION_JSON)
@Produces(MediaType.APPLICATION_JSON)
@Tag(name = "SlackService", description = "Mock endpoint for Slack service.")
public class SlackDummyService {
private static final Logger log = Logger.getLogger(SlackDummyService.class);
@Inject
ObjectMapper mapper;
@POST
public ObjectNode mockSlackEndpoint(String payload) {
log.infof("Received payload: %s", payload);
return mapper.createObjectNode().put("ok", true);
}
}
And if we implement the config options as above, we can use that service (instead of the live slack service) with a config change in application.properties;
%test.horreum.action.slack.host=localhost
%test.horreum.action.slack.port=8081
%test.horreum.action.slack.ssl=false
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.
Thanks for the sample. I'd considered mocking but while I knew I'd run across some API mocking somewhere, I couldn't find it again and at that point I decided I was better off posting something to review than spending more time searching.
ObjectMapper mapper = new ObjectMapper(); | ||
ObjectNode config = mapper.createObjectNode() |
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.
We should re-use Util.OBJECT_MAPPER
here
log.infof("Waiting for action to run"); | ||
int errors = 0; | ||
for (int i = 0; i < 10; i++) { | ||
try { | ||
Thread.sleep(1000); | ||
} catch (InterruptedException e) { | ||
} | ||
|
||
// Should these action errors be logged under dummyTest.id? The | ||
// ActionServiceImpl handler seems to always log under its argument | ||
// value, -1: but I still can't find them. | ||
List<ActionLog> logs = jsonRequest().auth().oauth2(getTesterToken()) | ||
.queryParam("level", PersistentLogDAO.ERROR).get("/api/log/action/-1") | ||
.then().statusCode(200) | ||
.contentType(ContentType.JSON).extract().body().jsonPath().getList(".", ActionLog.class); | ||
log.infof("[%d] Retrieved %d log entries", i, logs.size()); | ||
for (ActionLog l : logs) { | ||
if (l.type == SlackChannelMessageAction.TYPE_SLACK_MESSAGE) { | ||
log.infof("Found log for %d: %d, %s, %s: %s", l.testId, l.level, l.event, l.type, l.message); | ||
errors++; | ||
} | ||
} | ||
} | ||
log.infof("Found %d errors for %s on test %d", errors, SlackChannelMessageAction.TYPE_SLACK_MESSAGE, -1); |
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 we resolve the RLS issue, this will be a single check. and we can drop the loop.
However, we should test multiple scenarios with a mocked slack service, to test that valid and invalid requests to the slack API are handled appropriately
There is an issue with Row Level Security in the database not returning the log messages, I have opened an issue to tack this: #1353
I think we should mock the slack service in the tests. I included a simple example of how to do this in the review
I think this is fine for now. There is a much bigger exercise about making templates customizable, both for users and for integrations, I think we can/should handle the templating customizations in a different PR. The simplest way to handle this atm is within the BodyFormatters. Instead of creating a templates for;
The generic template could be; The git formatter could provide the correct link with;
The slack formatter could provide the correct link with;
Thanks for figuring this out, I have pinged @whitingjr to take a look as it was related to an issue opening the project with eclipse, but I am unsure why the ant plugin resolved this issue
This will need to be configured within the UI, please see https://github.com/Hyperfoil/Horreum/blob/master/horreum-web/src/domain/actions/ActionComponentForm.tsx#L111 on how this is done for the github actions Atm the UI is not pluggable for actions, so we need to add the UI logic directly into the component
Please can we have this information in a doc for slack integration in
|
@dbutenhof thank you for working on this :) |
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.
This will need to be configured within the UI,
Yes, and that was an issue I neglected to list. I'd started hacking around in the UI code but gave up when I was having trouble getting the (existing) action sheets working in my test environment. I'm not really a UI coder, but I do know just enough Javascript and React to be slightly dangerous (mostly to myself), so I can give it another try.
Please can we have this information in a doc for slack integration in docs/site/content/en/docs/Integrations?
Sorry, yes: another missing question, since at the time I'd looked I hadn't found much information on actions at all aside from a somewhat incomplete bit on the HTTP action.
|
||
@Override | ||
public void validate(JsonNode config, JsonNode secrets) { | ||
log.infof("Validating config %s, secrets %s", config, secrets); |
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.
I must have missed this when I was cleaning up: I'd meant to remove it, although it was really useful in initial testing to confirm that it had been called.
ObjectMapper mapper = new ObjectMapper(); | ||
ObjectNode body = mapper.createObjectNode(); |
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.
Ah, thanks: I'd expected there must be an object mapper already instantiated somewhere, but I failed to find it.
.setHost("slack.com") | ||
.setPort(443) | ||
.setURI("/api/chat.postMessage") | ||
.setSsl(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.
I'd initially toyed with various levels of URI configurability. For example, until I got the Slack auth token mechanism figured out I wasn't entirely sure that I could ignore Slack's subdomains for workspaces (e.g., redhat-internal.slack.com). In the end I determined there wasn't anything that needed to be user-configurable: but your suggestion of making it application configurable makes sense.
I think I'll follow the reasonable HTTP action model, accept a full URL and break it down using the URL
class rather than configuring each part separately, so it'd just be more like %test.horreum.action.slack.url=http://localhost:8081/api/slack
horreum-backend/src/test/java/io/hyperfoil/tools/horreum/svc/ActionServiceTest.java
Show resolved
Hide resolved
// If only ... | ||
// Assert.assertTrue("No slack action errors found", errors > 0); |
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.
Thanks for the sample. I'd considered mocking but while I knew I'd run across some API mocking somewhere, I couldn't find it again and at that point I decided I was better off posting something to review than spending more time searching.
If you would like any help with React+Typescript 😰 let me know and we can help with that part |
yes, unfortunately it is still on the backlog: #1177 |
@johnaohara ... Any status here? I'd feel better if my test could validate that the fake Slack post worked through standard APIs. (Although I did consider adding a Slack mock Anyway, the new commit has the UI working. (Or at least I can create global Actions.) |
@dbutenhof I have added @barreiro as a reviewer, he has volunteered to help get this PR merged. We are targeting the 0.13 release for this PR |
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.
@dbutenhof I added a few comments, but overall looks good.
just one last thing, after you address those please squash your commits into a single one
thanks for your work !!
- $ref: '#/components/schemas/GithubIssueCommentAction' | ||
- $ref: '#/components/schemas/GithubIssueCreateAction' | ||
- $ref: '#/components/schemas/GithubIssueCommentActionConfig' | ||
- $ref: '#/components/schemas/GithubIssueCommentActionConfig' |
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.
duplicate
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.
Generated because of the duplicate annotation in the DTO; but, thanks, good catch.
@DiscriminatorMapping(schema = SlackChannelMessageActionConfig.class, value = "slack-channel-message"), | ||
}, oneOf = { | ||
GithubIssueCommentActionConfig.class, | ||
GithubIssueCommentActionConfig.class, |
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.
duplicate
? { experimentResultToMarkdown: "Experiment result to Markdown" } | ||
: props.action.event === TEST_NEW | ||
? { testToSlack: "Test to Slack Markdown" } | ||
: {} |
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.
RUN_NEW
is missing
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.
There's no formatter for RUN_NEW
. Perhaps there should be. I decided that creating it was outside the scope of "implementing the slack action". (It affects GitHub issue comment, too, which allows selecting "run/new" while there's no applicable formatter, and the action can't actually be created.)
I find the implementation of the action form a bit awkward, and one of my complaints is this formatter popup when selecting the event really defines which formatter can apply anyway and it seems odd to require also selecting the formatter. I wrote up an issue about this, #1532
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.
I see two issues with the UI ... the formatters are one and another is the token that is sent as ******
to the server. since both are common to all the actions I say that we fix them in separate issues.
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 can get all the formatters for slack to be present in this PR that would be appreciated
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 would be easy enough to construct a new formatter, although we'd need to decide exactly what should be formatted for a new run and how to present it.
And if I do create a new formatter and use it here, it'll also be available for the GitHub issue comment action: if it includes a URL link, that'll be "badly formatted" for GitHub Markdown without
I decided it didn't make sense to get into all of that with this PR as it's not a new problem.
* retried asynchronously and we won't necessarily catch an error, but | ||
* it's sufficient for the other test cases. | ||
*/ | ||
Assert.assertEquals("Test case " + test_case.getKey() + " failed", test_case.getValue() ? 0 : 1, errors); |
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.
Assertions.assertEquals()
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.
Sure: any particular reason you prefer Assertions
over Assert
? There are so many different assertion packages floating around, and existing code seems to use several.
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.
no reason, just consistency with other assertions in the test
import jakarta.ws.rs.core.HttpHeaders; | ||
import org.eclipse.microprofile.config.inject.ConfigProperty; | ||
|
||
import org.jboss.logging.Logger; |
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.
unused
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.
I'd removed all the log output and apparently forgot to remove the import.
@@ -1 +1,2 @@ | |||
quarkus.test.enable-callbacks-for-integration-tests=true | |||
quarkus.http.test-port=8081 |
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.
this is not needed
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 not; I'll try again, but this solved one of the failures I'd run across during testing.
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.
Yes, I was able to remove it. Great. Whatever I ran into must have gone away. 😆
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.
just one last thing, after you address those please squash your commits into a single one
Sure. I usually don't like to squash during reviews because it complicates re-reviewing (and I'd already exposed this as a draft to John and Ståle). I'm used to routinely doing squash-and-merge anyway, so the extra commits don't generally matter.
thanks for your work !!
You're welcome. In a "previous life" I worked with Java EE exclusively for years (although Spring instead of Quarkus), but it's amazing how much my Java had gotten "out of shape" after working in Python for 3 years now. It's been an interesting experience.
- $ref: '#/components/schemas/GithubIssueCommentAction' | ||
- $ref: '#/components/schemas/GithubIssueCreateAction' | ||
- $ref: '#/components/schemas/GithubIssueCommentActionConfig' | ||
- $ref: '#/components/schemas/GithubIssueCommentActionConfig' |
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.
Generated because of the duplicate annotation in the DTO; but, thanks, good catch.
import jakarta.ws.rs.core.HttpHeaders; | ||
import org.eclipse.microprofile.config.inject.ConfigProperty; | ||
|
||
import org.jboss.logging.Logger; |
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.
I'd removed all the log output and apparently forgot to remove the import.
* retried asynchronously and we won't necessarily catch an error, but | ||
* it's sufficient for the other test cases. | ||
*/ | ||
Assert.assertEquals("Test case " + test_case.getKey() + " failed", test_case.getValue() ? 0 : 1, errors); |
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.
Sure: any particular reason you prefer Assertions
over Assert
? There are so many different assertion packages floating around, and existing code seems to use several.
@@ -1 +1,2 @@ | |||
quarkus.test.enable-callbacks-for-integration-tests=true | |||
quarkus.http.test-port=8081 |
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 not; I'll try again, but this solved one of the failures I'd run across during testing.
? { experimentResultToMarkdown: "Experiment result to Markdown" } | ||
: props.action.event === TEST_NEW | ||
? { testToSlack: "Test to Slack Markdown" } | ||
: {} |
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.
There's no formatter for RUN_NEW
. Perhaps there should be. I decided that creating it was outside the scope of "implementing the slack action". (It affects GitHub issue comment, too, which allows selecting "run/new" while there's no applicable formatter, and the action can't actually be created.)
I find the implementation of the action form a bit awkward, and one of my complaints is this formatter popup when selecting the event really defines which formatter can apply anyway and it seems odd to require also selecting the formatter. I wrote up an issue about this, #1532
The behavior of the action is similar to the existing GitHub actions, and largely a cookie-cutter clone. (Some common code could be extracted into a base class, but I resisted making unnecessary changes.) Essentially, it accepts a Slack service token, a Slack channel ID, and formatter, and posts the formatted message to the designated Slack channel. Creating a `slack-channel-message` action requires two "config" parameters and one "secrets" parameter: 1 The "channel" conf value 2 The Horreum "formatter" name 3 The "token" secret The Action will always post a "mrkdwn" Slack message using the markdown document generated by the Horreum formatter template. It will post using the name of the Slack app (e.g., "Horreum Server"). Note that there's some incompatibility between GitHub and Slack markdown syntax, specifically for links. Existing Horreum markdown templates for GitHub use the `[text](url)` syntax whereas Slack uses `<url|text>`. This suggests that Horreum may need to extend the templating classes to conditionalize link formatting based on the action context. Resolves issue Hyperfoil#838
Is this new? I've never seen it in local test runs. This seems to happen immediately after my new action test is torn down, so there could be a connection, but it's murky...
|
@dbutenhof this is a transient test failure that we see in the CI occasionally. We need to investigate the root cause, but it appears to be a race condition updating the Usually the issue resolves when the test is re-run. It is likely a bug in the test case that we need to fix |
Thanks. Transient errors are annoying, but in this case I'm mostly just happy to hear that it's not my transient error. 😆 |
@barreiro Where are we with a review of this PR? |
@dbutenhof thanks a lot for your contribution! |
@dbutenhof thank you very much for the contribution! |
Notes:
Test
object formatter with Slack markdown syntax. I have no idea whether a formatter forTest
objects might actually be useful.pom.xml
goes way back to our first debugging session when I was having Maven build problems...The behavior of the action is similar to the existing GitHub actions, and largely a cookie-cutter clone. (Some common code could be extracted into a base class, but I resisted making unnecessary changes.)
Essentially, it accepts a Slack service token, a Slack channel ID, and formatter, and posts the formatted message to the designated Slack channel.
On the Slack side, the setup required includes:
"xoxb-".
Creating a
slack-channel-message
action requires two "config" parameters and one "secrets" parameter:The Action will always post a "mrkdwn" Slack message using the markdown document generated by the Horreum formatter template. It will post using the name of the Slack app (e.g., "Horreum Server").
Note that there's some incompatibility between GitHub and Slack markdown syntax, specifically for links. Existing Horreum markdown templates for GitHub use the
[text](url)
syntax whereas Slack uses<url|text>
. This suggests that Horreum may need to extend the templating classes to conditionalize link formatting based on the action context.For testing (and as an example), this PR proposes a new formatter template for the
Test
class, using Slack link syntax.Resolves issue #838
Fixes Issue
Changes proposed
Check List (Check all the applicable boxes)