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

feat: Support appending to message attachments instead of replacing #74

Closed
wants to merge 4 commits into from

Conversation

nickelsen
Copy link
Contributor

For php release flow, we would like timestamps on the events, e.g. when build started, when deploy finished. To support that, I've added a flag called append-status-updates which will add new attachments with status updates incl timestamp to existing slack messages instead of replacing them with new status.

I've added two tests; one of the replacing functionality and one of the appending functionality
To make the tests work, I had to inject a layer between the message builder and the http client in SlackClient, which I call SlackHttpClient, which I then replace with a fake instance during testing, to assert on the messages and content that would be sent to slack.

Disclaimer: This is my first Kotlin PR ever, so I accept a lot of feedback. :D

@nickelsen nickelsen marked this pull request as ready for review September 30, 2024 10:42
)
}

private suspend fun getSlackMessageById(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These functions are moved to SlackHttpClient.

@@ -126,81 +121,54 @@ class SlackClient(
messageId: String? = null,
previousAttachments: List<SlackMessage.Attachment>? = null,
): SlackMessage {
val attachments = mutableMapOf<JobType, SlackMessage.Attachment>()
val attachments = if (appendAttachments) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the primary change of the PR, adding support for appending attachments instead of replacing them, depending on the input property.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this would break the updating of status in the notifier?

image

Specifically where it goes from In Progress to Success or am I wrong?

@nickelsen nickelsen closed this Nov 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants