Add `link` command and linking to jira on new issues #2922

tkoscieln · 2024-05-09T11:18:49Z

Pull Request Checklist

LecrisUT · 2024-05-09T14:41:14Z

Could use an example of how this works. Probably this needs to be abstracted a bit for more arbitrary issue providers. Are there no plans yet to strip out some of the tmt plugins in order to minimize the dependencies and tests?

tkoscieln · 2024-05-09T14:50:40Z

Could use an example of how this works. Probably this needs to be abstracted a bit for more arbitrary issue providers. Are there no plans yet to strip out some of the tmt plugins in order to minimize the dependencies and tests?

This is currently WIP, hence why it is in Draft. Will provide more information once the initial refactor suggested by @psss in a discussion will be implemented.

tkoscieln · 2024-05-10T11:58:37Z

Added a section to documentation which should explain what this change aims to introduce. Related Jira issue TT-262.

LecrisUT · 2024-05-10T12:41:07Z

Thanks for the clarification I kinda understand the setup, and I am more confused about it at the same time. The main idea is to add in the Issue links a link to the tmt test/plan/story that would fix a specific issue. The confusing part is the usage of the service field. This seems to imply that the tmt metadata is served somehow probably as teemtee/web? That part should be advertised a bit more if that is the only linkage that would be supported. What about general audience?

Another question I have is what happens with the links when a test/structure gets refactored. Could the tests be traced back through the git tree to see how it evolves and re-links?

happz · 2024-05-14T13:59:36Z

tmt/base.py

@@ -2862,23 +2903,30 @@ def tests(
            conditions: Optional[list[str]] = None,
            unique: bool = True,
            links: Optional[list['LinkNeedle']] = None,
-            excludes: Optional[list[str]] = None
+            excludes: Optional[list[str]] = None,
+            apply_command_line: Optional[bool] = True


If the parameter is only a boolean flag, and it never can be None, Optional is not needed. Check unique few lines above, the same case.

happz · 2024-05-14T13:59:58Z

tmt/base.py

@@ -3000,7 +3055,6 @@ def plans(

        if not Plan._opt('shallow'):
            plans = [plan.import_plan() or plan for plan in plans]
-


Unrelated change.

happz · 2024-05-14T14:02:10Z

tmt/cli.py

+@main.command(name='link')
+@pass_context
+@click.argument('link', nargs=1, metavar='[RELATION:]TARGET')
+@click.argument('names', nargs=-1, metavar='[NAME]...')


[NAME] - "name" of what? I'd suggest [TEST|PLAN|STORY]... instead.

happz · 2024-05-14T14:03:51Z

tmt/cli.py

+         link: list[str],
+         separate: bool,
+         ) -> None:
+    nodes: list[Union['tmt.base.Test', 'tmt.base.Plan', 'tmt.base.Story']] = []


tmt.base.Core would make the annotation simpler, unless there's a need for enumerating its child classes.

There was a issue in mypy throwing an error List item 0 has incompatible type "list[Test]"; expected "list[Core]" when the tmt.base.Core annotation was used, that is why it was changed to the Union

I'd give it a try, we should be able to address that issue in another way.

Thanks for helping with this @happz.

happz · 2024-05-14T14:04:13Z

tmt/utils.py

+def jira_link(
+        nodes: list[Union['tmt.base.Test', 'tmt.base.Plan', 'tmt.base.Story']],
+        links: 'tmt.base.Links',
+        separate: Optional[bool] = False) -> None:


A boolean flag, no need for Optional, it will never be set to None.

happz · 2024-05-14T14:08:03Z

tmt/utils.py

+        url: list[str] = []
+        # Get the fmf id of the object
+        if isinstance(tmt_object, tmt.base.Test):
+            fmfid = tmt_object.fmf_id


First, I believe you don't have to set fmf_id in every branch - all Core instances should have it, and indeed, all three lines are exactly the same, fmfid = tmt_object.fmf_id.

Second, you may use the class name to generate tmt_type, tmt_object.__class__.__name__.lower() should give you the same outcome.

Together, you should be able to replace this if with two lines handling all possible classes :)

happz · 2024-05-14T14:14:36Z

docs/guide.rst

+The ``type`` key specifies the type of the issue tracking service
+you want to link to (so far only Jira is supported).
+The ``server`` is the URL of said service. The ``service`` is the URL of the
+service that presents a tmt metadata in a human-readable form.


The difference between server and service is fairly minimal and got me thinking about which one is which. How about renaming these keys, e.g. server => website, jira-website (together with jira-token? Because a mere token - authenticate the user, sure, but against which service, the linking one or Jira?), service => something like tmt-link-service or something making it clear which service it is. At first, I thought it referred to Jira API...

Naming is hard. I agree that server and service are very similar. I'll try my brainstorm: If we call the config node issue-tracker it could suggest more that the data are related primarily to the issue tracking services so we could use url and token?

issue-tracker: - type: jira url: https://issues.redhat.com token: <YOUR_PERSONAL_JIRA_TOKEN> web: http://localhost:8000/

For the tmt web service I'd suggest to use web (consistent with the https://github.com/teemtee/web project) or tmt-web to make it even more specific. Nothing better comes to my mind right now.

happz · 2024-05-14T14:42:17Z

docs/guide.rst

+
+.. code-block:: shell
+
+    tmt link verifies:issues.redhat.com/browse/YOUR-ISSUE tests/core/smoke


Why not make schema part of the link from the beginning? Examples at https://tmt.readthedocs.io/en/stable/spec/core.html#link use it like it's nothing to be afraid of. verifies:https://issues.redhat.com/browse/YOUR-ISSUE would be complete, explicitly stated by the user, no space for tmt guess the wrong scheme. If I run my own ticket tracking service in my local network, without https, I wouldn't be able to use tmt link because it would be injecting https:// into my links.

Agreed, the full url should be used here.

happz · 2024-05-14T14:53:37Z

tmt/utils.py

+        joined_url = url + joined_url
+        # Ask for HTML format (WIP, can be changed once FE web server is implemented)
+        joined_url += '&format=html'
+        return joined_url


A couple of suggestions for URL generation: you spend too much time working out the low-level details, "manually" constructing the URL which is error-prone and easy to do incorrectly. I cannot recommend https://docs.python.org/3/library/urllib.parse.html#module-urllib.parse enough :)

create_url() is not creating URL, it's collecting its query components. The name is therefore misleading.

these components are pretty much key/value pairs, collect them as such. If the goal is to collect and construct URL query components, then use the best fitting data type for the goal, do not worry about the fact these components need to be encoded and merged into a string - such tasks are best performed at the boundary between your code and the outside world. The best-fitting data type is a simple dictionary:
tmt_type = tmt_object.__class__.__name__.lower() fmf_id = tmt_object.fmf_id url_params: dict[str, str] = { f'{tmt_type}-url': fmfid.url, f'{tmt_type}-name': fmfid.name } if fmf_id.path is not None: url_params[f'{tmt_type}-path'] = fmfid.path if fmf_id.ref is not None: url_params[f'{tmt_type}-ref'] = fmfid.ref return url_params

construct_url_from_list(url: str, url_parts: list[str]) takes url parameter which is unexpected - I'd suggest calling this one baseurl, it's a fairly common approach when you need to construct a URL from some starting website URL plus a path and/or parameters.

joined_url += '&format=html' - can we move this one to the create_url() function? It seems to be yet another parameter, url_params['format'] = 'html'.

https://docs.python.org/3/library/urllib.parse.html#urllib.parse.urlencode is exactly the helper we need to take parameters and convert them into a part of URL. It takes care of encoding weird characters, joining them with &, and all that stuff. When doing it manually, you always have to remember to put & at the right place, and you do not encode characters that need to be encoded, like spaces or /.

joined_url = url + joined_url - and if the service base URL does not end with /, this will produce invalid URL.

the same applies to parsing URL, see https://docs.python.org/3/library/urllib.parse.html#urllib.parse.urlparse - much better choice than split() and indexing. For one call, you get a named tuple and can inspect schema, hostname, all the components, with no chance of getting an index wrong by mistake.

As you can see, it looks trivial, but there are a lot of traps, encoding being the biggest one. Luckily, Python provides a very helpful library. Eventually, you can end up with something much simpler and safer, e.g.

def create_url_params(tmt_object: 'tmt.base.Core') -> dict[str, str]: tmt_type = tmt_object.__class__.__name__.lower() fmf_id = tmt_object.fmf_id url_params: dict[str, str] = { 'format': 'html', f'{tmt_type}-url': fmfid.url, f'{tmt_type}-name': fmfid.name } if fmf_id.path is not None: url_params[f'{tmt_type}-path'] = fmfid.path if fmf_id.ref is not None: url_params[f'{tmt_type}-ref'] = fmfid.ref return url_params def create_url(baseurl: str, url_params: dict[str, str]) -> str: return urllib.parse.urljoin(baseurl, urllib.parse.urlencode(url_params))

I'm not sure what's the purpose of service_url as a list, it seems to collect URL parameters for various nodes? if len(nodes) > 1 and not separate:, url_part = create_url(tmt_object=node) -> service_url.extend(url_part), in the next iteration, the list is empty and will get yet another string from new call to create_url(tmt_object=node)?

happz · 2024-05-14T15:18:13Z

tmt/utils.py

+    # Setup config tree
+    config_tree = tmt.utils.Config()
+    print(config_tree.fmf_tree.find('/user/linking').data)
+    # Linking is not setup in config, therefore user does not want to use linking


How do you know? :) The user just called tmt link, that seems to me like a solid argument that the user wanted to use linking :) Or the user called test create --link ..., then I guess the user is interested in linking... It's hard to tell from here, but most likely the opposite is true - jira_link would not get called if somebody didn't try to link things to Jira.

My two cents: the function does too many things. It takes care of links itself, but also prepares its own inputs, reads configuration, tries to look for a linking stanza in it, then picks a single link out of links - verifies only, no other relation is supported. It has so many inputs. I would recommend splitting it in two:

a function that focuses on creating links and nothing else. It's given a linking configuration, a single Link instance, and a list of nodes. And the separate, IIUIC. It has a simple task, talk to Jira, construct a linking service URL, and submit changes to Jira. No looking around, no external state to load or test, nothing to decide on its own, input-driven.

a function that manages these links, some kind of "ticket linking manager", something that's responsible for adding links to (various) services like Jira or Bugzilla or Trello, whatever. Give it nodes, give it a list of Link instances. It loads the configuration, finds what entries are there for linking, call the respective linking function for them and the given links - type: jira => jira_link(), raise an error for unknown type in the linking entry, unsupported. It may return the list of successfully processed links and leave it up to the CLI and base.py to decide whether it's fine or not that no Jira link was actually created, or it may raise an exception - or re-raise an exception raised by jira_link() if the linking fails.

I understand it's a WiP and a prototype, yet I'd recommend focusing on the separation of concerns. Otherwise, it will become harder and harder to extend and test.

As I think the separation to two functions is a good idea, I am not sure if the second function you mentioned should take a list of Link instances as a argument. AFAIK, currently, the idea was that we link in a one-to-many relation, several tmt objects can be linked to one issue. Also, supporting more types other than Jira is out of scope of this PR as well, but I agree that it is a good idea to refactor the code now to make it ready for adding more types in the future easily.

The approach outlined by @happz seems reasonable to me. Regarding the question around providing a list of Link instances: Currently the command line syntax for tmt link takes only a single link as input:

tmt link verifies:https://foo.com/a/b/c /plans/core /tests/smoke

It is true that tmt {test|plan|story} create commands can take multiple --link options so there seems to be use case also for multiple links. Thinking about it, should we change the design of tmt link to allow multiple links as well? Or are we ok with the currently proposed way?

How do you know? :) The user just called tmt link, that seems to me like a solid argument that the user wanted to use linking :) Or the user called test create --link ..., then I guess the user is interested in linking...

There are actually two parts of the linking action:

add the link to the test, plan or story metadata, this is now done by tmt * create only

link the web service url to the issue tracking service

It's perfectly fine if user does not have issues trackers defined in their config to only perform the first step. So calling tmt link with no config would also make sense for example for quickly updating metadata on disk for several tests at once.

Implementation of the second step should not be to much difficult as fmf already supports a comfortable way to modify existing metadata on disk using a with statement. Here's an example for inspiration:

tmt/tmt/export/nitrate.py

Lines 438 to 439 in e6a0964

with test.node as data:

data["extra-nitrate"] = nitrate_case.identifier

happz · 2024-05-14T15:18:36Z

tmt/base.py

@@ -1261,6 +1261,15 @@ def _get_template_content(template: str, template_type: str) -> str:
                force=force,
                logger=logger)

+            if links.get('verifies') and dry is False:
+                test = Tree(


tests instead of test.

happz · 2024-05-14T15:18:52Z

tmt/base.py

@@ -1920,6 +1936,15 @@ def create(
                force=force,
                logger=logger)

+            if links.get('verifies') and dry is False:
+                plan_list = Tree(


plans instead of plan_list.

happz · 2024-05-14T15:19:08Z

tmt/base.py

@@ -2628,6 +2660,15 @@ def create(
                force=force,
                logger=logger)

+            if links.get('verifies') and dry is False:
+                story_list = Tree(


And stories instead of story_list :)

lukaszachy · 2024-05-14T19:43:35Z

pyproject.toml

@@ -39,6 +39,7 @@ dependencies = [              # F39 / PyPI
    "requests>=2.25.1",       # 2.28.2 / 2.31.0
    "ruamel.yaml>=0.16.6",    # 0.17.32 / 0.17.32
    "urllib3>=1.26.5, <3.0",  # 1.26.16 / 2.0.4
+    "jira"


Could we make it a separate package for the 'link' functionality and add dependency just there?

Could you elaborate please? How would that look?

Write it as a separate project. It can be in a different repo like what I'm working on with tmt-cmake or it can live within the current git repo, e.g. hatch contains hatchling as a sub-project.

w.r.t. how to add functionality to tmt, you can use project.entry-points."tmt.plugin".

Should note that we still haven't pinned down how to make it available for testing-farm ¹, but let's investigate it when it's closer to production.

Side-note, this feature requires teemtee/web doesn't it? It would be nice to incorporate these 2 together. An abstract link interface should be available as @happz mentioned so this might be a bit more complicated to have it in a completely separate repo right now (not impossible, I do that in tmt-cmake)

Footnotes

https://github.com/teemtee/tmt/issues/2758#issuecomment-1995473532 ↩

psss · 2024-05-17T11:14:11Z

pyproject.toml

@@ -65,6 +65,9 @@ report-polarion = [
    "tmt[report-junit]",
    "tmt[export-polarion]",
    ]
+link-jira = [
+    "jira>=3.2.0",
+    ]


The new optional dependency should be also created as an rpm package. The relevant spec file lines are here:

tmt/tmt.spec

Lines 29 to 31 in e6a0964

%pyproject_extras_subpkg -n tmt export-polarion

%pyproject_extras_subpkg -n tmt report-junit

%pyproject_extras_subpkg -n tmt report-polarion

psss

Thanks a lot for working on this! Overall looks very good. Added some comments and suggestions.

psss · 2024-05-17T11:18:14Z

docs/guide.rst

@@ -398,6 +398,41 @@ Sometimes you forget something, or just things may go wrong and
 you need another try. In such case add ``-f`` or ``--force`` to
 quickly overwrite existing files with the right content.

+.. _linking_tmt_objects:
+
+Linking tmt objects


Suggested change

Linking tmt objects

Linking Issues

Everything here's about tmt objects, the issues are the interesting part here. Please, update the link anchor above as well.

psss · 2024-05-17T11:22:13Z

docs/guide.rst

+
+.. code-block:: shell
+
+    tmt link verifies:issues.redhat.com/browse/YOUR-ISSUE tests/core/smoke


Agreed, the full url should be used here.

psss · 2024-05-17T12:01:44Z

docs/guide.rst

+    tmt link verifies:issues.redhat.com/browse/YOUR-ISSUE tests/core/smoke
+
+For this feature to be enabled, you have to create a configuration node in a
+configuration tree. Once the configuration is present, it automatically enables


I was thinking about about the proper name for the config node. Currently we only have /user/plan which is used in tmt try for pointing to default plan(s). Trying to imagine more... I guess there could be /user/options for storing default command line options like /user/options/verbose or /user/options/color or similar.

The list of issue tracking servers does not fit there completely I'd say. What about storing them in the root of the config tree as /linking? Instructions for setting up the config could be easier (once we have the config section itself documented, but let's not block on that now):

Create a file ~/.config/tmt/linking.fmf with the list of issue tracking services and corresponding tokens:

- type: jira server: https://issues.redhat.com service: http://localhost:8000/ token: <YOUR_PERSONAL_JIRA_TOKEN>

Or, perhaps, if this is supposed to be list of issues trackers, we could/should call it issue-tracker.fmf instead?

psss · 2024-05-17T12:16:23Z

docs/guide.rst

+The ``type`` key specifies the type of the issue tracking service
+you want to link to (so far only Jira is supported).
+The ``server`` is the URL of said service. The ``service`` is the URL of the
+service that presents a tmt metadata in a human-readable form.


Naming is hard. I agree that server and service are very similar. I'll try my brainstorm: If we call the config node issue-tracker it could suggest more that the data are related primarily to the issue tracking services so we could use url and token?

issue-tracker: - type: jira url: https://issues.redhat.com token: <YOUR_PERSONAL_JIRA_TOKEN> web: http://localhost:8000/

For the tmt web service I'd suggest to use web (consistent with the https://github.com/teemtee/web project) or tmt-web to make it even more specific. Nothing better comes to my mind right now.

psss · 2024-05-17T12:17:42Z

tmt/cli.py

@@ -1160,6 +1160,9 @@ def plans_lint(
 @option(
    '--finish', metavar='YAML', multiple=True,
    help='Finish phase content in yaml format.')
+@option(
+    '--link', metavar='[RELATION:]TARGET', multiple=True,
+    help='Link to the relevant issues.')


Suggested change

help='Link to the relevant issues.')

help='Link created plan to the relevant issues.')

psss · 2024-05-17T12:43:05Z

tmt/utils.py

+
+
+def import_jira() -> None:
+    """ Import polarion Python Jira library """


Suggested change

""" Import polarion Python Jira library """

""" Import Python Jira library """

psss · 2024-05-17T12:44:19Z

tmt/utils.py

+        from jira import JIRA
+    except ImportError:
+        raise GeneralError(
+            "Install 'tmt+link-jira' to use the Jira linking")


Suggested change

"Install 'tmt+link-jira' to use the Jira linking")

"Install 'tmt+link-jira' to use the Jira linking.")

See the recommendations for the consistent message format.

psss · 2024-05-17T12:46:29Z

tmt/utils.py

+    """ Link the object to Jira issue and create the URL to tmt web service """
+    import_jira()
+
+    def create_url_params(tmt_object: 'tmt.base.Core') -> dict[str, str]:


Please, add a docstring.

psss · 2024-05-17T13:02:36Z

tmt/utils.py

+    # Setup config tree
+    config_tree = tmt.utils.Config()
+    print(config_tree.fmf_tree.find('/user/linking').data)
+    # Linking is not setup in config, therefore user does not want to use linking


The approach outlined by @happz seems reasonable to me. Regarding the question around providing a list of Link instances: Currently the command line syntax for tmt link takes only a single link as input:

tmt link verifies:https://foo.com/a/b/c /plans/core /tests/smoke

It is true that tmt {test|plan|story} create commands can take multiple --link options so there seems to be use case also for multiple links. Thinking about it, should we change the design of tmt link to allow multiple links as well? Or are we ok with the currently proposed way?

psss · 2024-05-17T13:10:27Z

tmt/utils.py

+    # Setup config tree
+    config_tree = tmt.utils.Config()
+    print(config_tree.fmf_tree.find('/user/linking').data)
+    # Linking is not setup in config, therefore user does not want to use linking


How do you know? :) The user just called tmt link, that seems to me like a solid argument that the user wanted to use linking :) Or the user called test create --link ..., then I guess the user is interested in linking...

There are actually two parts of the linking action:

add the link to the test, plan or story metadata, this is now done by tmt * create only

link the web service url to the issue tracking service

It's perfectly fine if user does not have issues trackers defined in their config to only perform the first step. So calling tmt link with no config would also make sense for example for quickly updating metadata on disk for several tests at once.

Implementation of the second step should not be to much difficult as fmf already supports a comfortable way to modify existing metadata on disk using a with statement. Here's an example for inspiration:

tmt/tmt/export/nitrate.py

Lines 438 to 439 in e6a0964

with test.node as data:

data["extra-nitrate"] = nitrate_case.identifier

Add link command and linking to jira on new issues.

fe0d5ff

psss changed the title ~~Add link command and linking to jira on new issues.~~ Add link command and linking to jira on new issues May 9, 2024

Refactor link creation, fix type issues, add docs

931fe16

happz reviewed May 14, 2024

View reviewed changes

lukaszachy reviewed May 14, 2024

View reviewed changes

tkoscieln added 4 commits May 15, 2024 13:05

Change names in base.py

b867a61

Add small fixes, rework url construction, move dependency to opt

15bd302

Add check for Jira dependency import

b6dd1d7

Fix unit tests

09c5590

psss reviewed May 17, 2024

View reviewed changes

psss requested changes May 17, 2024

View reviewed changes

Fix creation of a path

888754e

		@@ -3000,7 +3055,6 @@ def plans(

		if not Plan._opt('shallow'):
		plans = [plan.import_plan() or plan for plan in plans]


		.. code-block:: shell

		tmt link verifies:issues.redhat.com/browse/YOUR-ISSUE tests/core/smoke

	with test.node as data:
	data["extra-nitrate"] = nitrate_case.identifier

	%pyproject_extras_subpkg -n tmt export-polarion
	%pyproject_extras_subpkg -n tmt report-junit
	%pyproject_extras_subpkg -n tmt report-polarion

	help='Link to the relevant issues.')
	help='Link created plan to the relevant issues.')



		def import_jira() -> None:
		""" Import polarion Python Jira library """

	""" Import polarion Python Jira library """
	""" Import Python Jira library """

	"Install 'tmt+link-jira' to use the Jira linking")
	"Install 'tmt+link-jira' to use the Jira linking.")

Add link command and linking to jira on new issues #2922

Are you sure you want to change the base?

Add link command and linking to jira on new issues #2922

Conversation

tkoscieln commented May 9, 2024 • edited

LecrisUT commented May 9, 2024

tkoscieln commented May 9, 2024 • edited

tkoscieln commented May 10, 2024

LecrisUT commented May 10, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lukaszachy May 14, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Footnotes

Choose a reason for hiding this comment

psss left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Add `link` command and linking to jira on new issues #2922

Add `link` command and linking to jira on new issues #2922

tkoscieln commented May 9, 2024 •

edited

tkoscieln commented May 9, 2024 •

edited

lukaszachy May 14, 2024 •

edited