-
Notifications
You must be signed in to change notification settings - Fork 45
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
base: master
Are you sure you want to change the base?
slack-support #60
Conversation
d8f0f7f
to
c77012e
Compare
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 again for creating this PR.
Not sure if this is ready for review yet, but I added some preliminary comments.
sources/src/main/java/com/google/solutions/jitaccess/core/adapters/SlackAdapter.java
Show resolved
Hide resolved
sources/src/main/java/com/google/solutions/jitaccess/core/adapters/SlackAdapter.java
Outdated
Show resolved
Hide resolved
sources/src/main/java/com/google/solutions/jitaccess/core/adapters/SlackAdapter.java
Outdated
Show resolved
Hide resolved
sources/src/main/java/com/google/solutions/jitaccess/web/ApiResource.java
Outdated
Show resolved
Hide resolved
sources/src/main/java/com/google/solutions/jitaccess/web/ApiResource.java
Outdated
Show resolved
Hide resolved
@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 🙌 |
cb8dcdb
to
0bbf642
Compare
<artifactId>slack-api-client</artifactId> | ||
<version>1.27.3</version> | ||
</dependency> | ||
<dependency> |
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.
Isn't this pulled in automatically as a transitive dependency?
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.
Was assuming that too, but wasn't the case. Ended up crashing with every notification
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 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.
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 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 = |
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.
Depending on the type of notification, the message should be different. Could we use the same template mechamism as in the SMTP adapter?
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.
will fix, it's not HTML though, so might have to do it a bit differently.
sources/src/main/java/com/google/solutions/jitaccess/web/RuntimeEnvironment.java
Show resolved
Hide resolved
Is this feature operational, can it be tested? |
@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. |
Hello @adriantr What environment variables we need to use here to have slack notifications |
@adriantr we are trying to integrate JIT with Slack for approval, any update when this will be available? |
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. |
Adding slack support