-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
migrate connector to power platform for msteams #3921
Conversation
a527868
to
0ecc664
Compare
Could you please help review this PR? Do you know why the test run successfully in Github action but failed in CircleCI? |
config/notifiers.go
Outdated
@@ -803,9 +802,8 @@ type MSTeamsConfig struct { | |||
WebhookURL *SecretURL `yaml:"webhook_url,omitempty" json:"webhook_url,omitempty"` | |||
WebhookURLFile string `yaml:"webhook_url_file,omitempty" json:"webhook_url_file,omitempty"` | |||
|
|||
Title string `yaml:"title,omitempty" json:"title,omitempty"` | |||
Summary string `yaml:"summary,omitempty" json:"summary,omitempty"` |
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 don't think we can remove this without a deprecation notice and subsequent deprecation period (for example, scheduled to be removed in the next 3 minor releases).
There are a number of projects that use Alertmanager such as cortex and mimir, and removing this field will break Alertmanager configurations for tenants in those systems. The reason for this is Alertmanager uses yaml.UnmarshalStrict
, which will fail if a configuration file contains a summary
field.
Instead, we either need to keep it and find a use for it – or keep it, don't use it and document that it's deprecated so it can be removed at a later date.
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 for your review.
Good point. I'll keep it and document that it's deprecated.
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.
Hey @zhan9san @grobinson-grafana , are we going to have the feature released that supports msteams-power-platform anytime soon ?
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.
The summary
still exists in configuration, but the loaded summary value would not be used in payload.
If there is no concern, feel free to resolve this conversation.
Technically, this is an breaking change. Keep that in mind. Not sure if its worth to setup an msteamsv2 connector and deprecate the old one to increase the awareness for end users. |
Thanks for your review. I don't think v2 is necessary because the original strategy would be abandoned by msteams in a short time window. The incompatible migration is a must.
Wave 1 – effective August 15th, 2024: All new Connector creation will be blocked within all clouds Wave 2 – effective October 1st, 2024: All connectors within all clouds will stop working |
At least, It worth to mentation that the docs that the connector now requires a Power Automate license. I'm aware for that situation, however many users maybe not and my create issues here at AM, if wave 2 starts. |
it makes sense. I am on vacation, and will be back one week later. I can add this notice in doc when I am back, if it's not urgent. |
I'm also desperately waiting for this PR to be merged. Thanks for your help and dedication. |
Signed-off-by: Jack <jack4zhang@gmail.com>
Signed-off-by: Jack <jack4zhang@gmail.com>
b571eca
to
18ce823
Compare
Signed-off-by: Jack <jack4zhang@gmail.com>
18ce823
to
7d485f7
Compare
The license has been updated in doc. Do you still have any concern? @simonpasquier Could you please help review this PR? |
The doc is clear. That helps. You can optionally mention, that as of October 1st, 2024, classical Teams webhooks are not longer supported by Microsoft. including a link to the official blog post. That may help users, that are not fully familiar with Teams. There are a lot users that may had to integrate msteams as receiver, without been in that eco system. |
In the meantime the timeline has been changed by Microsoft. See the update at the top at https://devblogs.microsoft.com/microsoft365dev/retirement-of-office-365-connectors-within-microsoft-teams/:
|
At least, they continue to work for more year. From my point of view, the existing receiver should not be changed and a new one should be introduced. |
Instead of introducing a new receiver, I prefer to add an optional flag in the configuration, such as If Let me know your thoughts. |
I'm not sure about this because it will mean we need to transition users a second time to remove the flag once power automate becomes the only working option and we want to remove the flag. |
I get your point. How about using webhook_connector flag? true: webhook connector After the webhook connector is retired, the users would have to migrate to power auto platform. The we can remove the flag at any time. It's a one time change for power auto platform user. For users who still want to use webhook connector, they have to set it to true explicitly after upgrade, and migrate to power auto platform next year. |
Thats an breaking change. What the issue with and v2 notifier which brings the best possible user experience? Why so complicated? From maintainer point of view, it's much easier to just remove an whole notifier in a year. Creating an notifier which support both feel counterintuitive. For example, the summary property has no effect, if power flows are used. This my confused people. Then, new property needs to be introduced which must mark instantly as deprecated. Having both logics in one notifier increases the complexity on code. |
It's okay. I'll add a new receiver. |
I hope you don’t mind, but I would greatly appreciate another opportunity to discuss this. There are two services: Webhook Connector and Power Automate. I attempted to add a new receiver, but I found that detecting the service via URL is much more straightforward.
If we detect it automatically, the only thing users need to do is to change the URL. |
I'm afraid I don't see how that makes it more straightforward? What do you plan to do with the summary field when the URL matches the regex |
Is this still being worked on, or should i take this over? |
Feel free to take it. Thanks. |
This can be closed in favor of #3920. |
Fix #3920
The
summary
is marked as DEPRECATED in this PR because there is no appropriate field in Request Body Schema.Besides, adding support of more flexible Adaptive Cards is not covered in this PR. It may be in another PR.
Migration plan:
Test result
Reference
https://support.microsoft.com/en-us/office/creating-a-workflow-from-a-channel-in-teams-242eb8f2-f328-45be-b81f-9817b51a5f0e
https://devblogs.microsoft.com/microsoft365dev/retirement-of-office-365-connectors-within-microsoft-teams/
an example json can be used in Postman