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

Provide the ability to send events to Google Chat #149

Merged
merged 5 commits into from
Mar 1, 2021
Merged

Provide the ability to send events to Google Chat #149

merged 5 commits into from
Mar 1, 2021

Conversation

djexp
Copy link
Contributor

@djexp djexp commented Feb 27, 2021

By essentially replicating the implementations for Teams and Slack a specific implementation for Google Chat has been developed and tested.

The are two points of note:

  1. I've had to filter out 'Progressing' events as these seemed to replace the previous message within Google Chat. I couldn't easily find out why. However the output in chat is very similar to Discord so why this system does not display these messages is a mystery?

  2. I've included the event timestamp as 'meta-data' within the message. Other implementation have not done this, so I'm happy to remove it for consistency sake if that is preferred.

Finally. I am not a Go developer, this is actually my first time coding in it. I like it, it's very clean. But I cut my teeth in C, C++, Java and beyond. So my apologies for any novice mistakes.

Signed-off-by: Darren Everley <darren.everley@xeropoint.co.uk>
By essentially replicating the implementations for Teams and Slack a specific implementation for Google Chat has been developed and tested.

Signed-off-by: Darren Everley <darren.everley@xeropoint.co.uk>
…ly making the change to the generated artefact.

Signed-off-by: Darren Everley <darren.everley@xeropoint.co.uk>
Copy link
Member

@phillebaba phillebaba left a comment

Choose a reason for hiding this comment

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

I see that you are setting a proxy URL, is this to deal with authentication? I am not to sure that this is the way to go as an official solution. Why can you not pass a token to the provider?

internal/notifier/google_chat.go Show resolved Hide resolved
@djexp
Copy link
Contributor Author

djexp commented Mar 1, 2021

In answer to both questions:

Firstly the proxy was added for consistency with all of the existing notification providers and is passed to the underlying HTTPClient during connection initialisation. I presume for accessing the internet for certain corporate networks. So it is not added for authentication, tokens are part of the URL as I believe they are for at least Slack and Teams.

Secondly, yes the embedded HTML markup is part of the API spec. See this link: https://developers.google.com/hangouts/chat/reference/message-formats/cards#card_text_formatting

@phillebaba
Copy link
Member

@djexp sorry I was comparing the provider too much to the git providers. From my part this looks good, as the text formatting is documented by google.

I just want to get a second opinion from @stefanprodan before merging.

@phillebaba
Copy link
Member

@djexp I just missed that you have not added the new provider to the factory. You need to add a case for the google chat provider in factory.go.

func (f Factory) Notifier(provider string) (Interface, error) {

@djexp
Copy link
Contributor Author

djexp commented Mar 1, 2021

You're right. I've got that locally. Somehow failed to commit it.

I'll take care of that now.

Thanks for being thorough.

Signed-off-by: Darren Everley <darren.everley@xeropoint.co.uk>
@phillebaba phillebaba self-requested a review March 1, 2021 10:33
Copy link
Member

@phillebaba phillebaba left a comment

Choose a reason for hiding this comment

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

LGTM

@djexp
Copy link
Contributor Author

djexp commented Mar 1, 2021

I'm no expert with Github Actions, but is it normal for actions to be queued for hours like these have been?

@stefanprodan
Copy link
Member

@djexp GH seems stuck, can you please force push the last commit, maybe it will unblock the actions.

@stefanprodan
Copy link
Member

I've included the event timestamp as 'meta-data' within the message. Other implementation have not done this, so I'm happy to remove it for consistency sake if that is preferred.

Please remove it, the timestamp should be available in the chat.

…p is 'identical' or as near as can be.

Signed-off-by: Darren Everley <darren.everley@xeropoint.co.uk>
Copy link
Member

@stefanprodan stefanprodan left a comment

Choose a reason for hiding this comment

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

LGTM

Thanks @djexp 🏅

@phillebaba phillebaba merged commit c7c64c0 into fluxcd:main Mar 1, 2021
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.

3 participants