MM-17818 Adding automatic registration of autolinks for JIRA projects. #453

crspeller · 2020-01-23T00:18:03Z

Adds automatic registration of autolinks of JIRA projects converting JIRA tags like MM-1234 to clickable links and pasted links to clickable tags. These are the same links used on community.

This PR requires: mattermost-community/mattermost-plugin-autolink#85

Some notes to be fixed before merge or in follow up PRs:

It won't build publicly until the autolink PR is merged
Only works for cloud instances. @mickmister @levb Looking for advice on how to enable this for server instances.
Needs to act when all plugins are initialized or when the autolink plugin is enabled for the first time. Currently misses the second case and uses a sleep for the first. This will be fixed by: https://mattermost.atlassian.net/browse/MM-21763
Autolinks won't be updated when projects are created on the JIRA server. Workaround is to disable and re-enable the jira plugin to cause the re-registration to happen. @aaronrothschild follow up ticket?
Tests.

levb

I think the biggest 2 changes I request are:

move the link (struct) creation to the Instance interface
consider adding Jira server link support (@DHaussermann or @jfrerich can provide the info on the URL structure)

and naming nits, as usual

server/plugin.go

levb · 2020-01-23T11:40:46Z

server/plugin.go

+				continue
+			}
+
+			if err := p.InstallAutolinkForCloudInstance(jci); err != nil {


1/5 s/Install/Configure/; 0/5 s/InstallAutolinkForCloudInstance/configureCloud/

OT: IMO reading a function name, one should need to switch to a "parsing" mode, names should read naturally, with the emphasis on the client (caller). I started thinking using this pattern more, to keep the names cleaner. Also helps keepinf the vocabulary consistent. 1/5 we should rename Link to Autolink in the autolink plugin itself before too long.

type autolinkAPI struct {} func (*autolink) Configure(...link.Link) {...} ... autolink := NewAutolinkAPI() err = autolink.Configure(ji.Autolinks()...)

(OT: I recently learned that it's ok to use a _ or even nothing for the receiver! )

It seems like an "Install" to me since we are sending data to a separate system?

reading a function name, one should need to switch to a "parsing" mode, names should read naturally, with the emphasis on the client (caller)

+1000 You read code way more then you write it.

1/5 we should rename Link to Autolink

Ya I struggled with the name there. I think you are right it should be Autolink. Will change.

autolink.Install(...*autolink.Autolink) wfm

0/5 To me, install has the connotation of "install and enable the plugin as needed" which is not true, but I can see your POV as well. Maybe autolink.Add(links)?

@levb Switched to using a new autolink client to address this feedback.

server/plugin.go

levb · 2020-01-23T11:52:00Z

RE: It won't build publicly until the autolink PR is merged - gomod to the rescue? Got import github.com/mattermost/mattermost-plugin-autolink/v2? :) -- not for this PR, but want to point out the upcoming dependency hell, and circular dependencies appearing.

Consider starting a separate versioned package for all P2P types? Or each plugin having to declare a sub-package with no dependencies that others can depend on?

crspeller · 2020-01-23T15:55:53Z

@levb How will gomod help? Nothing can be picked up until it's merged.

Yes. Circular dependencies will have to be avoided. In this case it's fine because autolink should know nothing about any other plugin. But I did factor out the link stuff into a separate package anyway.
When building this inter-plugin stuff, we should be sure to define our interfaces well and include them in separate packages as necessary.

Separate versioned package is probably overkill in this situation, but I could see it being useful in the future.

hanzei · 2020-01-25T16:53:47Z

@levb How will gomod help? Nothing can be picked up until it's merged.

You can use the commit on the feature branch as a dependency, e.g. with go get github.com/mattermost/mattermost-plugin-autolink@383545a0f9abbc17ad99f1271ce93ef7b296c7af to use mattermost-community/mattermost-plugin-autolink@383545a

Sure, if there are following commits that break the API there is an problem.

mickmister

Awesome job @crspeller! Just some nits on error messages

server/plugin.go

Co-Authored-By: Michael Kochell <mjkochell@gmail.com>

levb · 2020-01-30T14:38:37Z

go.mod

@@ -8,9 +8,12 @@ require (
 	github.com/dghubble/oauth1 v0.5.0
 	github.com/dgrijalva/jwt-go v3.2.0+incompatible
 	github.com/google/uuid v1.1.1
+	github.com/mattermost/mattermost-plugin-autolink v0.0.0-00010101000000-000000000000


We should start defining proper versions in our go.mod's?

levb · 2020-01-30T14:42:19Z

Ready to un-draft this, or wait for the other PRs? @crspeller

I guess CI is failing...

crspeller · 2020-01-30T22:31:32Z

Will un-draft when the other PR is merged and I have updated this one.

DHaussermann

I am having issues building this locally even after getting the branch for the corresponding functionality in the other repo.

Please merge and I will test from master and create follow-up issues as needed.

aaronrothschild · 2020-02-12T13:05:48Z

Just for tracking - I created a follow up ticket for registering automatically: #472

hanzei · 2020-05-13T08:39:50Z

server/plugin.go

+	}
+
+	client := autolinkclient.NewClientPlugin(p.API)
+	if err := client.Add(installList...); err != nil {


@crspeller Is the code checking anywhere if the autolink plugin is enabled and running? Or does this line just fail if autolink is not running?

Yup it just fails. No need to check as checking is probably just as expensive as just trying and failing.

/giphy fire and forget :)

I'm not sure if it's about cost. This code is only executed once on startup.

My concerns are mostly that admins might be confused on the log warning.

I agree with @hanzei. Maybe we can add something to the error that says "Make sure the autolink plugin is installed in order to auto-create links to your Jira issues."

Adding automatic registration of autolinks for JIRA projects.

8ae1b1e

crspeller added 1: PM Review Requires review by a product manager 2: Dev Review Requires review by a core committer 3: QA Review Requires review by a QA tester labels Jan 23, 2020

crspeller requested review from aaronrothschild, levb, mickmister and DHaussermann January 23, 2020 00:18

crspeller mentioned this pull request Jan 23, 2020

MM-17818 Adding API for adding/replacing links. mattermost-community/mattermost-plugin-autolink#85

Merged

levb reviewed Jan 23, 2020

View reviewed changes

crspeller added 2 commits January 27, 2020 09:23

Autolink plugin ID constatnt

adea899

Using autolink client and some renaming.

28abaf4

crspeller requested a review from levb January 27, 2020 18:03

mickmister suggested changes Jan 28, 2020

View reviewed changes

server/plugin.go Outdated Show resolved Hide resolved

server/plugin.go Outdated Show resolved Hide resolved

server/plugin.go Outdated Show resolved Hide resolved

Wording fixes

25d689f

Co-Authored-By: Michael Kochell <mjkochell@gmail.com>

crspeller requested a review from mickmister January 28, 2020 16:04

mickmister approved these changes Jan 28, 2020

View reviewed changes

levb approved these changes Jan 30, 2020

View reviewed changes

levb removed the 2: Dev Review Requires review by a core committer label Jan 30, 2020

DHaussermann approved these changes Feb 3, 2020

View reviewed changes

DHaussermann removed the 3: QA Review Requires review by a QA tester label Feb 3, 2020

Using actual autolink instead of local copy.

d812396

crspeller marked this pull request as ready for review February 3, 2020 18:44

crspeller merged commit 879250a into master Feb 3, 2020

crspeller deleted the mm-17818 branch February 3, 2020 18:48

aaronrothschild removed the 1: PM Review Requires review by a product manager label Feb 12, 2020

aaronrothschild mentioned this pull request Feb 12, 2020

Auto-detect new projects in Jira and update Auto-link registration #472

Closed

mickmister mentioned this pull request Feb 12, 2020

Prompt MM user to resolve ticket when their Github PR is merged #342

Closed

hanzei added the 4: Reviews Complete All reviewers have approved the pull request label Mar 7, 2020

jfrerich mentioned this pull request Mar 31, 2020

Bump version to v2.4.0 #510

Closed

11 tasks

jfrerich mentioned this pull request Apr 28, 2020

Bump version to 2.4.0 #530

Merged

hanzei reviewed May 13, 2020

View reviewed changes

hanzei mentioned this pull request May 13, 2020

Register autolinks for GitHub URLs mattermost/mattermost-plugin-github#269

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MM-17818 Adding automatic registration of autolinks for JIRA projects. #453

MM-17818 Adding automatic registration of autolinks for JIRA projects. #453

crspeller commented Jan 23, 2020

levb left a comment

levb Jan 23, 2020 •

edited

Loading

crspeller Jan 23, 2020

levb Jan 25, 2020

levb Jan 25, 2020 •

edited

Loading

crspeller Jan 27, 2020

levb commented Jan 23, 2020

crspeller commented Jan 23, 2020

hanzei commented Jan 25, 2020

mickmister left a comment

levb Jan 30, 2020

levb commented Jan 30, 2020 •

edited

Loading

crspeller commented Jan 30, 2020

DHaussermann left a comment

aaronrothschild commented Feb 12, 2020

hanzei May 13, 2020

crspeller May 13, 2020

levb May 13, 2020

hanzei May 14, 2020 •

edited

Loading

mickmister May 14, 2020

MM-17818 Adding automatic registration of autolinks for JIRA projects. #453

MM-17818 Adding automatic registration of autolinks for JIRA projects. #453

Conversation

crspeller commented Jan 23, 2020

levb left a comment

Choose a reason for hiding this comment

levb Jan 23, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

levb Jan 25, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

levb commented Jan 23, 2020

crspeller commented Jan 23, 2020

hanzei commented Jan 25, 2020

mickmister left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

levb commented Jan 30, 2020 • edited Loading

crspeller commented Jan 30, 2020

DHaussermann left a comment

Choose a reason for hiding this comment

aaronrothschild commented Feb 12, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hanzei May 14, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

levb Jan 23, 2020 •

edited

Loading

levb Jan 25, 2020 •

edited

Loading

levb commented Jan 30, 2020 •

edited

Loading

hanzei May 14, 2020 •

edited

Loading