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

slack-support #60

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

adriantr
Copy link
Contributor

@adriantr adriantr commented Mar 22, 2023

Adding slack support

Copy link
Collaborator

@jpassing jpassing left a comment

Choose a reason for hiding this comment

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

Thanks again for creating this PR.

Not sure if this is ready for review yet, but I added some preliminary comments.

@adriantr
Copy link
Contributor Author

@jpassing thanks for taking a look. Wasn't yet ready, no, but should be tomorrow after some live testing! I think I adressed all your points in the new approach 🙌

@adriantr adriantr force-pushed the slack-support branch 3 times, most recently from cb8dcdb to 0bbf642 Compare March 31, 2023 09:54
<artifactId>slack-api-client</artifactId>
<version>1.27.3</version>
</dependency>
<dependency>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Isn't this pulled in automatically as a transitive dependency?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Was assuming that too, but wasn't the case. Ended up crashing with every notification

Copy link
Collaborator

Choose a reason for hiding this comment

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

I wonder if we even need the slack-api-client and okhttp dependency -- it seems like all that needs to happen here is something like:

    var payload = (...)
    HttpTransport.newTransport()
        .createRequestFactory()
        .buildPostRequest(
            new GenericUrl("https://slack..."),
            new JsonHttpContent(new GsonFactory(), payload))
        .execute();

We don't have to do this in this PR -- but I'm always looking for ways to reduce the number of dependencies, so I might want to do it as a follow-up task.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sounds reasonable, what's needed for now it's basically this:

POST https://hooks.slack.com/services/T00000000/B00000000/XXXXXXXXXXXXXXXXXXXXXXXX
Content-type: application/json
{
    "text": "Hello, world."
}

public void sendNotification(Notification notification) throws NotificationException {
Preconditions.checkNotNull(notification, "notification");

var message =
Copy link
Collaborator

Choose a reason for hiding this comment

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

Depending on the type of notification, the message should be different. Could we use the same template mechamism as in the SMTP adapter?

Copy link
Contributor Author

@adriantr adriantr Apr 11, 2023

Choose a reason for hiding this comment

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

will fix, it's not HTML though, so might have to do it a bit differently.

@michalbagrowski
Copy link

Is this feature operational, can it be tested?

@adriantr
Copy link
Contributor Author

adriantr commented May 30, 2023

@michalbagrowski the ambition is to refactor it throughout june due to lack of time recently. I am however running it in our production env currently, so feel free to fork my repo.

@rajat2911
Copy link

Hello @adriantr What environment variables we need to use here to have slack notifications

@rh-sph
Copy link

rh-sph commented Mar 12, 2024

@adriantr we are trying to integrate JIT with Slack for approval, any update when this will be available?

@jpassing
Copy link
Collaborator

Note that you can also configure JIT Access to post notifications to a Pub/Sub topic. You could listen to those notifications and relay them to Slack or other systems.

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.

None yet

5 participants