search

atc0005 / go-teams-notify

A package to send messages to a Microsoft Teams channel
MIT License
100 stars 17 forks source link

Private teamsMessage Type in SendWithContext Method #285

Closed nikoksr closed 3 months ago

nikoksr commented 3 months ago

Hi there, notify author here! We've been in contact before about a similar issue.

A recently opened, very friendly issue in Notify brought to my attention that the O365 connectors within Teams will be deprecated. Since notify is still using deprecated members of your library like goteamsnotify.MessageCard, I began with fixing this. While doing so I noticed some issues when trying to abstract the SendWithContext method.

The Issue

func (*goteamsnotify.TeamsClient) SendWithContext(ctx context.Context, webhookURL string, message goteamsnotify.teamsMessage) error

goteamsnotify.teamsMessage appears to be a private type.

Impact on Notify

The main problem we're facing is that we can no longer abstract and mock the teams library due to the private teamsMessage type. This is particularly important for Notify because we rely heavily on mocking for our testing strategy. With over 30 external messaging platforms integrated into Notify, conducting end-to-end tests for each one isn't feasible. Mocking allows us to effectively test our integrations without the need for actual external connections.

This is the interface we used to use to abstract your teams library in Notify:

type teamsClient interface {
    SendWithContext(ctx context.Context, webhookURL string, webhookMessage teams.MessageCard) error
    SkipWebhookURLValidationOnSend(skip bool) teams.API
}

Now, if switching to the latest state of go-teams-notify, we'd have to do this:

type teamsClient interface {
    SendWithContext(ctx context.Context, webhookURL string, webhookMessage teams.teamsMessage) error
    SkipWebhookURLValidationOnSend(skip bool) *teams.TeamsClient
}

And at that point, we're trying to externally access a private member of your library which of course leads to a compile-time error:

compiler: teamsMessage not exported by package goteamsnotify

While we could add a whole wrapper layer in Notify around your library to work around this issue, it would introduce a lot of complexity and make maintenance more challenging. I'd really like not to.

Possible Solutions

Please let me know if there's anything unclear and thank you for your continued maintenance of this project.

Cheers, Niko

atc0005 commented 3 months ago

Hi @nikoksr,

Possible Solutions:

  • Make teamsMessage public

I'm interested in testing that change.

For your project, what command do you run to regenerate the mocks for the service/msteams files?

I'm not familiar with mockery and want to make sure I'm following the same steps that you would.

I see how you have it listed here:

//go:generate mockery --name=snsSendMessageAPI --output=. --case=underscore --inpackage

but since I don't see the same here:

type teamsClient interface {
// ...

I wasn't sure if that indicated that you handcrafted the file or if I was just overlooking something.

From what I can tell I can see that this file is generated, so I just assumed that I was overlooking the steps for generating it.

nikoksr commented 3 months ago

@atc0005 thanks for the quick response.

Nice catch, that's got to be a leftover because I thought I had removed all those go:generate directives. We recently upgraded to the latest version of mockery which uses a config based approach (you can see the config at the root of the repo). Running make mock should be sufficient to (re)generate the mocks.

Let me know if there's anything else, I can help you with. And thanks for taking a look at this.

atc0005 commented 3 months ago

@nikoksr

Thanks for that. I ran make mock in a test branch to regenerate the mocks. Ran it several times as I fumbled about. :)

Test changes here in a fork of your repo:

For simplicity I did the following:

The commits are messy (some unrelated changes included), log messages not so great, etc.

I tried to deep link directly to relevant bits.

After all changes the tests pass:

$ go test -v ./service/msteams/
=== RUN   TestMSTeams_Send
=== PAUSE TestMSTeams_Send
=== CONT  TestMSTeams_Send
=== RUN   TestMSTeams_Send/Successful_send_to_single_webhook
=== PAUSE TestMSTeams_Send/Successful_send_to_single_webhook
=== RUN   TestMSTeams_Send/Successful_send_to_multiple_webhooks
=== PAUSE TestMSTeams_Send/Successful_send_to_multiple_webhooks
=== RUN   TestMSTeams_Send/Teams_client_error
=== PAUSE TestMSTeams_Send/Teams_client_error
=== CONT  TestMSTeams_Send/Successful_send_to_single_webhook
=== CONT  TestMSTeams_Send/Teams_client_error
=== CONT  TestMSTeams_Send/Successful_send_to_multiple_webhooks
--- PASS: TestMSTeams_Send (0.00s)
    --- PASS: TestMSTeams_Send/Teams_client_error (0.00s)
    --- PASS: TestMSTeams_Send/Successful_send_to_single_webhook (0.00s)
    --- PASS: TestMSTeams_Send/Successful_send_to_multiple_webhooks (0.00s)
PASS
ok      github.com/nikoksr/notify/service/msteams       0.010s

From what I can tell exporting TeamsMessage (without its internals) is enough to make this work. Feedback is welcome.

If nothing further needs adjusting in this project I'll plan to release that change as a follow-up (e.g., v2.12.0) to the upcoming v2.11.0 release.

nikoksr commented 3 months ago

Hi @atc0005, you went above and beyond with that, thank you so much.

Sounds great! Do I see it correctly that there's nothing I need to explicitly do to fix this issue? I'll refactor Notify's teams service accordingly once v2.12.0 is live, which will remove all the deprecated bits of your library that we're still using. There's nothing I need to add for it to support workflow hooks, right?

I checked this PR of yours and from what I can tell, the most breaking thing that changes is the URL pattern validation. Which would make this more of a user concern than mine, if I understand it correctly.

atc0005 commented 3 months ago

Hi @nikoksr,

you went above and beyond with that, thank you so much

You're welcome.

the most breaking thing that changes is the URL pattern validation

Perhaps you didn't mean it that way, but I'd argue that's not a breaking change. The previous pattern validation behavior continues to work as before, only now an additional default pattern is applied to permit workflow connector URLs to validate alongside O365 connector URLs.

Do I see it correctly that there's nothing I need to explicitly do to fix https://github.com/nikoksr/notify/issues/839? I'll refactor Notify's teams service accordingly once v2.12.0 is live

Now that you've looked over the changes and don't see any glaring omissions, I'll proceed with applying the necessary changes to the development branch and tag a pre-release so you and others can test further (if desired).

Thanks for the feedback.

atc0005 commented 3 months ago

I'll proceed with applying the necessary changes to the development branch and tag a pre-release so you and others can test further (if desired).

@nikoksr,

New tag:

nikoksr commented 3 months ago

Created a PR in Notify where I successfully implemented the fixed teams service using v2.12.0-alpha.1. Will merge once it gets fully released.

Please let me know if there's anything left to clarify. And thanks again for your quick help, Adam :)

atc0005 commented 3 months ago

@nikoksr,

You're welcome.

Thanks for the CC on that PR. It was good to see the full set of needed changes.

Created a RC tag to invite/encourage feedback from others before tagging a stable release:

atc0005 commented 3 months ago

@nikoksr new release: