-
Notifications
You must be signed in to change notification settings - Fork 85
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
added basic/simple adaptive cards support #358
added basic/simple adaptive cards support #358
Conversation
28ea028
to
2913c95
Compare
2913c95
to
0723040
Compare
|
||
import java.util.List; | ||
|
||
public interface Action { |
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.
Action
is very generic and very popular class name, how about making ConnectorAction
or similar to avoid mismatch in class importing?
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 take CardAction
since the new way is not a Connector
anymore, but a PowerAutomate Workflow
.
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.
Done
|
||
void setName(String name); | ||
|
||
void setTarget(List<String> target); |
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.
setTargets
since this takes list as a parameter
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.
in general i agree, but currently consistent to https://github.com/jenkinsci/office-365-connector-plugin/blob/master/src/main/java/jenkins/plugins/office365connector/model/PotentialAction.java#L57
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.
then I believe both should be updated to be consistent
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.
Done
|
||
@Override | ||
public void setThemeColor(final String cardThemeColor) { | ||
|
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.
empty method?
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.
yep, haven't used cardThemeColor
(because the AdaptiveCard
format doesn't really support color - ok with workarounds - it just a fixed enum https://github.com/microsoft/AdaptiveCards/blob/main/schemas/1.4.0/adaptive-card.json#L1494), but too much depends in the old MessageCard
on having this method ... so i left it intentionally empty (meaning: not enough time to do necessary refactoring).
|
||
import java.util.List; | ||
|
||
public class AdaptiveCardAction implements jenkins.plugins.office365connector.model.Action { |
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.
why full package name here?
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
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.
Done
|
||
@Override | ||
public void setTarget(final List<String> target) { | ||
target.stream().findFirst().ifPresent(this::setUrl); |
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.
applies only to first element, why is that ?
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.
Old PotentialAction
works internally with list https://github.com/jenkinsci/office-365-connector-plugin/blob/master/src/main/java/jenkins/plugins/office365connector/model/PotentialAction.java#L34 cause the format expects that https://learn.microsoft.com/en-us/outlook/actionable-messages/message-card-reference#openuri-action.
But the AdaptiveCard
action doesn't https://adaptivecards.io/explorer/Action.OpenUrl.html.
So that's why internally url
(single) is used.
As far as i can see, the whole code doesn't add more than one anyway. So a bigger refactoring should/could be, work internally with single attribute and only modify the getTarget
in MessageCard
to return a list to fulfil the MessageCard JSON format.
f366899
to
e815b95
Compare
two basic integration tests added to have at least "some" coverage. |
What I understand from #355 with new approach we need to configure how to convert information from Jenkins into MS Teams even so I am not sure if we need to convert to adaptive card |
In can be any Card format which ideally is not deprecated or declared legacy. As far as i understand https://learn.microsoft.com/en-us/outlook/actionable-messages/message-card-reference, the current implemented format is not recommended anymore
Indeed the same thing says
Which is a) not true (from other applications we were sending Adaptive Cards since more than a year to the mentioned connectors) and b) they retired the connectors. So IMHO the best way to go forward is, to use AdaptiveCard format. |
Hello @markush81 , is it possible to set adaptiveCards as default? "Incoming Webhook" already has native support, it would not impact the current connector and would help in the migration to power automate. |
Technically we can, but i am not sure if there is agreement for what the plugin should do in future. |
@markush81 Is there any update on the status of this? |
Since i am not maintainer, i am also waiting for review and merge. IMHO i have done the requested changes a while ago. |
@damianszczepanik It does look like the requested changes have been made. Is there anything else? |
e786256
to
371f5d8
Compare
One concern I have is to how to configure the hook. Based on the comments from developers I'm not even sure if we need this change as I found conclusion that with powerautomate you need to translate what is received via webhook into MS Teams message. So concluding it would be best to update README file as well so this is clear how to configure hook with new addaptive cards and also so I'm able to test it before it gets merged. |
In the retirement article https://devblogs.microsoft.com/microsoft365dev/retirement-of-office-365-connectors-within-microsoft-teams/, they mention the migration:
I haven't found any good other description, most create it custom and parse the JSON and so on ... but we never did this, we migrated all our webhooks with exactly this template and didn't need to do anything further than that. |
That change would be quite useful since manually converting the json in powerautomate is a bit tedious, and doesn't scale that well when you have many hooks |
OK, this sounds like question for the answer I didn't get the answer so far! |
One piece of information, adaptive cards work in the new flow with powerautomate and in the current Incoming Webhook. As a suggestion, it would be interesting to leave the default adaptive cards, aiming at the future that the current standard is discontinued. |
371f5d8
to
d53d6c5
Compare
|
||
public class Webhook extends AbstractDescribableImpl<Webhook> { | ||
|
||
public static final Integer DEFAULT_TIMEOUT = 30000; | ||
private static final Logger log = LoggerFactory.getLogger(Webhook.class); |
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.
added but not used
} | ||
} | ||
|
||
private String color(final Result result) { |
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 guess this should be removed later ?
@SuppressFBWarnings(value = "SS_SHOULD_BE_STATIC") | ||
private final String version = "1.4"; | ||
@SerializedName("msTeams") | ||
private final MsTeams msteams = new MsTeams(); |
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.
if you use msTeams
then @SerializedName
should be useless and name will be more like Java convention
|
||
public class MsTeams { | ||
|
||
@SuppressFBWarnings(value = "SS_SHOULD_BE_STATIC") |
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.
since width
is not a final I believe this annotation is not needed here
public class Payload { | ||
|
||
@SuppressFBWarnings(value = "SS_SHOULD_BE_STATIC") | ||
private String type = "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.
same here about annotation: annotation or static
public class MessageCard implements Card { | ||
|
||
private String summary; | ||
private String themeColor = "3479BF"; |
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.
is it valid for adaptive cards?
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.
Adaptive Cards and colors are different, there is either a fixed enum which can be used, as done here jenkins.plugins.office365connector.model.adaptivecard.AdaptiveCard#color
or do sth. like Semaphore UI does https://github.com/semaphoreui/semaphore/blob/develop/services/tasks/templates/microsoft-teams.tmpl#L44
@@ -24,7 +24,7 @@ | |||
import org.junit.Test; | |||
import org.mockito.MockedStatic; | |||
|
|||
public class ActionableBuilderTest { | |||
public class ActionablePotentialActionBuilderTest { |
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.
there is no class ActionablePotentialActionBuilder
so there is something wrong with class name
@@ -0,0 +1,27 @@ | |||
{ |
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.
name of this file does not pass exactly naming convention others
@markush81 I did review one more time. I have left a few comments but they are not blocking merging this change to master branch. This pull request is significant and valuable. |
@@ -201,8 +213,8 @@ | |||
return FormUtils.formValidateUrl(value); | |||
} | |||
|
|||
public FormValidation doCheckGlobalUrl(@QueryParameter String value) { | |||
if(StringUtils.isNotBlank(value)) { | |||
public FormValidation doCheckGlobalUrl(@QueryParameter String value) { |
Check warning
Code scanning / Jenkins Security Scan
Stapler: Missing POST/RequirePOST annotation Warning
@@ -201,8 +213,8 @@ | |||
return FormUtils.formValidateUrl(value); | |||
} | |||
|
|||
public FormValidation doCheckGlobalUrl(@QueryParameter String value) { | |||
if(StringUtils.isNotBlank(value)) { | |||
public FormValidation doCheckGlobalUrl(@QueryParameter String value) { |
Check warning
Code scanning / Jenkins Security Scan
Stapler: Missing permission check Warning
Adjust most of the them.
I am fine with the plan.
We have this version with my PR running since i created it, in our installation - so kinda live-testing.
I can't promise anything, but of course will do my best in case after merge issues occur. |
Thanks a lot to you guys :) |
@markush81 @kikmon version 5.0.0 has been published |
I'm a little bit confused how to proceed with an upgrade to 5.0.0.
|
As far as I know, this MR added the parameter adaptiveCards in the declarative pipeline too, so you should be good. |
What do i need to change in a groovy script to send a success/unstable message.
As far as i gathered, color is not supported anymore, ss how would i indicate started/success/failure? An updated documentation would be very nice :) |
OK, I think I figured out the upgrade path:
At least this is how it behaves on my environment and Microsoft 365 tenant. |
Hi, An example with Post to a channel when a webhook request is received Power Automate Workflow would be:
Regards |
@rdrgporto would you prepare pull request with updated README file about adaptive cards |
@markush81 In #355 (comment) @ViliusS has mentioned that the plugin does not support credentials what is benefit of having adaptive cards but is still not supported by the plugin. How did you fix that problem? |
@damianszczepanik maybe I was not very clear with my comments in another thread. Adaptive Card support in the plugin is working and fine. The issue I was discussing is that someone said that Workflow trigger URL is public, hence insecure. But it was also the case for Incoming Webhook or Jenkins connectors. Nothing has changed regarding security on that part. BTW, Microsoft actually updated their documentation today: https://devblogs.microsoft.com/microsoft365dev/retirement-of-office-365-connectors-within-microsoft-teams/ 2025-02-01 - All connectors with old webhook URL will stop working. Users must update webhook URLs. So far as I tried, at this point, only Incoming Webhook connector is working with the updated URL. Jenkins connector doesn't work and it doesn't matter which Card format you send. I suspect it needs to be updated somehow to accept new URL format, but I doubt Microsoft will ever do that. Especially considering that that connector only accepts legacy MessageCard messages. Thinking about AdaptiveCard support further I would vote to enable it by default now and point plugin users to upgrade instructions, or do it after 2025-02-01 which will probably mark the end of Jenkins connector on MS side anyway. Also probably plugin naming could be revised again as AdaptiveCards are now implemented by Cisco too. Maybe more implementations will follow. I would vote for something more generalized, like |
There is no authentication, but @ViliusS already commented i see. |
Coloring in AdaptiveCards works with a fixed set (or adding background images). So far the plugin determines the color based on build result https://github.com/jenkinsci/office-365-connector-plugin/blob/master/src/main/java/jenkins/plugins/office365connector/model/adaptivecard/AdaptiveCard.java#L40. |
It would be a example like that in README.md? (after Pipeline post section): Pipeline post section with Adaptive Cards
Thanks in advance, Regards |
Sounds like a good example of how to use new approach - please create PR with updated README file |
@rdrgporto Are you planning to raise a PR with changes to README about the change? If not, would be happy to help create it. Thanks! |
Hi, @damianszczepanik and @aksh05 I have created a Pull Request: Thanks in advance, Regards |
Testing done
Add starting point for AdaptiveCard Support because of Microsofts announcement.
Only manually tested in installation where i am working.
The PR is just meant as starting point to fully implement what is needed (unfortunately i have not the time to add tests and all variants, like check encoding of provided message).
Submitter checklist
-- Retirement of Office 365 connectors within Microsoft Teams #355
-- New "workflows" based webhooks are failing #353
-- Add adaptive cards support #297