chaoss / grimoirelab-elk

GNU General Public License v3.0
59 stars 121 forks source link

[mattermost] Allow parsing standard mattermost urls #998

Closed Alch-Emi closed 2 years ago

Alch-Emi commented 3 years ago

With the addition of https://github.com/chaoss/grimoirelab-perceval/pull/748 to grimoirelab-perceval, it is now possible to call perceval using mattermost team & channel names. This means that it is possible to accept a standard mattermost URL in grimoirelab-elk. This PR adds support for this ability, while maintaining support for "legacy" repos in the old format.

This also adds a degree of validation to URLs, since they are now matched by regex, and improperly formatted URLs will fail to match. I think this could be an opportunity to add an informative error message linking to the docs, but I wasn't able to find a place where the URL format is documented to link to.

mattermost.py:125 has an unfilled (TODO) where I think it would be good to link documentation. I'd be happy to fill it in, and also update whatever documentation is currently available, but I'm not sure where to look.

coveralls commented 3 years ago

Pull Request Test Coverage Report for Build 1187212580


Totals Coverage Status
Change from base Build 1186824695: 0.02%
Covered Lines: 8693
Relevant Lines: 10563

💛 - Coveralls
Alch-Emi commented 3 years ago

Does anyone know if there's documentation for the URL formats that I could link to here?

Alch-Emi commented 3 years ago

Before I rebase, I'd like to just make sure that that the reference to the docs that I mentioned earlier is filled out, so that the error message with the TODO in it doesn't accidentally get committed. I still don't know of any place where the URLs are documented though. Can you point me somewhere I could find that?

sduenas commented 2 years ago

Before I rebase, I'd like to just make sure that that the reference to the docs that I mentioned earlier is filled out, so that the error message with the TODO in it doesn't accidentally get committed. I still don't know of any place where the URLs are documented though. Can you point me somewhere I could find that?

I think we don't have any kind of documentation. What we have is the perceval backend and that's all. I made that backend long ago so I don't remember the details. However, if I remember well, URLs were set concatenating the host address and the channel id because there wasn't a real URL. What I'm sure is that there's no white space in between both. You can check the original commit and the tests about it: https://github.com/chaoss/grimoirelab-perceval/commit/3f04b1c7b35ef04454f2c1070e9b3b522f03a0ad

Alch-Emi commented 2 years ago

Yeah, the space existed only in the ELK format. I'm quite confident that's the current URL format that it expects, even before this commit adds a new form. You can see this in the current test data, although the parameters are transformed before they are passed to perceval. It's unfortunate that there is no place that this is documented though. I'll just remove the reference for now.

Alch-Emi commented 2 years ago

Alright, just pushed the change and rebased, should be good to merge if everyone is still okay with it.