Add 'link' as a core attribute

psss commented 4 years ago

Looks good to me. I've just adjusted the wording a bit. @lukaszachy, could you please confirm the changes are ok?

psss commented 4 years ago

One more thought though, or rather a little brainstorm: In the fmf repository I've experimented already some time ago with an attribute coverage which could link to stories covered by the test. Something like that also allows to select tests by coverage. Would it make sense to have something like that with support for external trackers as well?

lukaszachy commented 4 years ago

Renamed to 'coverage'. Feel free to reword or squash.

kkaarreell commented 4 years ago

In the summary you say "Issues or stories related to the test". There is a difference between related (e.g. bug identified due to a failing test case) and covered in the sense of "test designed to test bugfix xy" (ignoring now a case when that one test is testing just part of the bug scenario). If this is really about bug 'coverage' as the text suggest I think you should change the Summary to something more clear.

psss commented 4 years ago

Good point, @kkaarreell. The summary comes from the original idea of the issue attribute which could contain (probably) any related bugs and features. So the question is: Do we want this attribute to be used only for bugs/features/stories the test completely covers/verifies? That would mean a separate attribute related or something like this for other relation types. Or we use issue for any issues and coverage for verifying relation.

Or we support adding a relation type to each link. It could look like this:

issue:
  - link: https://github.com/psss/tmt/issue/123
    type: verify
  - link: https://bugzilla.redhat.com/show_bug.cgi?id=1
    type: relate

There would be a default type so that default relations could be still specified in a concise way:

issue:
  - https://github.com/psss/tmt/issue/123
  - https://bugzilla.redhat.com/show_bug.cgi?id=1

Or even like this:

issue: https://github.com/psss/tmt/issue/123

Internally tmt would always convert it into a list of dictionaries so that standard format could be parsed. Multiple types could give us some nice extensibility for the future. End of brainstorming.

kkaarreell commented 4 years ago

While adding a type would be more extensible for the future, it seems not very user friendly. As a user I would prefer either 'verifies' for the verification relationship (now ignoring the situation of partial-coverage) or 'relates_to' for the other one. What about supporting both, e.g. 'verifies: url' would be automatically transformed to 'link: url; type: verify' ?

lukaszachy commented 4 years ago

How about making coverage official tmt attribute And add link (or similar) to express any other relationship?

psss commented 4 years ago

Summary from today's discussion: As the first step let's introduce a flexible, multipurpose link attribute which could contain various relations. It would be a list of strings or dictionaries. Single value supported similarly as with require or recommend. A couple of examples:

link:
  - url: https://github.com/psss/tmt/issue/123
    type: verify
  - name: /some/fmf/node
    type: verify

There would be a default type of the relation. The question is still which one. From my point of view, when not specified, the default should be rather generic/general, so I'd propose relate or similar. Short version then could look like this:

link:
  - url: https://github.com/psss/tmt/issue/123
  - name: /some/fmf/node

Or even this?

link:
  - https://github.com/psss/tmt/issue/123
  - /some/fmf/node

And finally the shortest version?

link: https://github.com/psss/tmt/issue/123

Possible relations could be:

verify
relate
block
duplicate
parent
child

End of brainstorm :-)

psss commented 4 years ago

One more note: As part of the dictionary implementation we could also use the fmf identifier to reference arbitrary fmf nodes. That means allow url, ref, path and name keys as defined by the spec.

lukaszachy commented 3 years ago

Adding 'must' priority as we need to track feature/issue <-> test relationship somehow.

lukaszachy commented 3 years ago

To extend last @psss' idea- how about allowing relation name (value of type) as key name? Or maybe even force it.

Allowed values will be: a) url to issue (jira, bugzilla, github...) as string b) fmf name (in same fmf root) as string, starting with '/' c) fmf-id dictionary

Shortest (implying 'verify' is the default value):

link: https://bugzilla.redhat.com/show_bug.cgi?id=1

link: 
 - https://bugzilla.redhat.com/show_bug.cgi?id=1

link:
 /fmf/id

link:
 - /fmf/id

link:
 - url: https://github.com/psss/tmt.git
   name: /spec/test/link

When one is explicit about the relation

link: 
 verify: https://bugzilla.redhat.com/show_bug.cgi?id=1

link: 
 - verify: https://bugzilla.redhat.com/show_bug.cgi?id=1

link:
 verify: 
   url: https://github.com/psss/tmt.git
   name: /spec/test/link

link:
 - verify: 
     url: https://github.com/psss/tmt.git
     name: /spec/test/link

lukaszachy commented 3 years ago

Possible relations could be:

verify

relate

block

duplicate

parent

child

I'll add depend (as in test/story depends on 3rd party issue)

lukaszachy commented 3 years ago

What do you think about current PR? Link as a core attribute

packit-as-a-service[bot] commented 3 years ago

Congratulations! One of the builds has completed. :champagne:

:warning: Please note that our current plans include removal of these comments in the near future (at least 2 weeks after including this disclaimer), if you have serious concerns regarding their removal or would like to continue receiving them please reach out to us. :warning:

You can install the built RPMs by following these steps:

sudo yum install -y dnf-plugins-core on RHEL 8
sudo dnf install -y dnf-plugins-core on Fedora
dnf copr enable packit/psss-tmt-215
And now you can install the packages.

Please note that the RPMs should be used only in a testing environment.

kkaarreell commented 3 years ago

@lukaszachy Just to clarify.. All relations are one-directional, right? I mean, if a requirement is a parent of another requirement, then the another element should be a child. That could apply also to relations like block (vs is_blocked_by), duplicate (is_duplicated_by), verify (is_verified_by)... Of course, the problem here is that the information about the relation may not be available (since it is defined from the other direction) but in case both objects would be within one object tree, would this bi-directional mapping make sense? If not, I would probably avoid using conflicting relations like parent and child as it would be unclear which one to use.

lukaszachy commented 3 years ago

@lukaszachy Just to clarify.. All relations are one-directional, right? I mean, if a requirement is a parent of another requirement, then the another element should be a child. That could apply also to relations like block (vs is_blocked_by), duplicate (is_duplicated_by), verify (is_verified_by)... Of course, the problem here is that the information about the relation may not be available (since it is defined from the other direction) but in case both objects would be within one object tree, would this bi-directional mapping make sense? If not, I would probably avoid using conflicting relations like parent and child as it would be unclear which one to use.

Well, my intentions to use this attribute within TMT are zero. What I need to express:

Test verifies bug (hopefully we can get reverse link somehow into bugzilla as well, but that is a completely different task)
Test verifies jira/github issue/...
Test is blocked by other bugzilla / jira (e.g. it is disabled until something else is fixed)

So the rest is kind of 'why not, it might be useful to someone'.

Do I understand correctly, that if link "target" is again TMT object then pair link will be automatically added (probably in memory only, maybe linter can be used to write this to .fmf files)? That seems to to be useful if someone uses TMT for stories as well.

Pair being: depend - block parent - child verify - is_verified_by? duplicate - duplicate? relate - relate?

lukaszachy commented 3 years ago

For me MVP is 'verify' and 'depend' relations as one directional link.

jscotka commented 3 years ago

Hi, what about adding some short and more descriptive aliases like:

verify: {BZ|bz}#123456

What will have same meaning as wrote:

link:
 - verify: 
     url: https://bugzilla.redhat.com/show_bug.cgi?id=123456
     name: /spec/test/link

I understand that you want to have it as generic as possible and very powerful. But it would be perfect to have it as short as possible for most of the cases and I think that it will cover most of the usage of this link attribute.

psss commented 3 years ago

@lukaszachy, thanks for preparing the spec. Summary from the stakeholder meeting:

The outlined specification looks good in general
Proposed relations fine, just a little discussion about the exact form, e.g. depend versus depends
The default relation to be used when not explicitly specified should be relate
Discussed whether relation should be a key or stored as type: relation
Support for shortcuts such as BZ#123456 can be added later (no priority for now)

The following counterparts should be used for bidirectional links:

relate - relate (default)
depend - block
parent - child
verify - test
duplicate - duplicate

The discussion about the way how relation should be stored slightly inclined to the shorter variant. Let's agree on the final decision here by sharing pros/cons and voting.

psss commented 3 years ago

Relation in the key

# local fmf name
link:
    verify: /stories/cli/test/lint

# remote fmf id
link:
    verify:
        url: https://github.com/psss/tmt/
        name: /stories/cli/test/lint

# external url
link:
    verify: https://bugzilla.redhat.com/show_bug.cgi?id=1920176

# multiple links
link:
  - verify: /stories/cli/common/verbose
  - relate: https://bugzilla.redhat.com/show_bug.cgi?id=1920176
  - depend:
        url: https://github.com/psss/tmt/
        name: /stories/cli/test/lint
    note: Does not cover the --fix option yet

psss commented 3 years ago

Relation in the type

# local fmf name
link:
    type: verify
    name: /stories/cli/test/lint

# remote fmf id
link:
    type: verify
    url: https://github.com/psss/tmt/
    name: /stories/cli/test/lint

# external url
link:
    type: verify
    url: https://bugzilla.redhat.com/show_bug.cgi?id=1920176

# multiple links
link:
  - type: verify
    name: /stories/cli/common/verbose
  - type: relate
    url: https://bugzilla.redhat.com/show_bug.cgi?id=1920176
  - type: depend
    url: https://github.com/psss/tmt/
    name: /stories/cli/test/lint
    note: Does not cover the --fix option yet

psss commented 3 years ago

From the two options how to explicitly define the relation I would choose the shorter one which stores relation in the key. What I like especially about this implementation is the concise arrangement for multiple links:

link:
  - verify: /stories/cli/common/verbose
  - relate: https://bugzilla.redhat.com/show_bug.cgi?id=1920176
  - depend: /tests/core/smoke

psss commented 3 years ago

Regarding the relation naming an meaning I would propose to understand the key as the description of what is on the right side:

link:
    parent: this-is-my-parent
    child: this-is-my-child
    block: this-is-what-blocks-me
    depend: this-depends-on-me
    test: test-which-covers-me
    verify: bug-which-i-verify

It's true that the block and depend relations can be understood the other way as well, and verify does not completely fit here, but consistently interpreting the key as a "heading" could help. Not sure, please share your thoughts.

WOnder93 commented 3 years ago

It's true that the block and depend relations can be understood the other way as well, and verify does not completely fit here, but consistently interpreting the key as a "heading" could help. Not sure, please share your thoughts.

I think blocks and depends-on would be much more intuitive than depend and block, plus it matches the Bugzilla terminology most of us are trained to parse already :) Similarly I would vote for verifies instead of verify, covered-by instead of test, and maybe even parent-of/child-of instead of parent/child.

My 2 cents...

thrix commented 3 years ago

I would vote more for ideas from @WOnder93 in regards to naming of the relations, so :+1: from me

t184256 commented 3 years ago

Imagine you know nothing about this PR and you encounter

link: https://github.com/psss/tmt/issues/207

Nothing in this line conveys that the relationship is verify. Unless you look it up in the docs, you would assume it's something relevant (relates).

Because of that, I propose to change the default to relates and be explicit about verify.

psss commented 3 years ago

Because of that, I propose to change the default to relates and be explicit about verify.

Yes, that has been already discussed on the meeting and included in the summary. We'll update the spec once we agree on the remaining points.

psss commented 3 years ago

So here's the summary of the relation names proposed by @WOnder93:

link:
  - blocks: issue blocked by me
  - depends-on: issue which blocks me
  - verifies: issue verified by me
  - covered-by: test which covers me
  - parent-of: this is my child
  - child-of: this is my parent

Keeping consistency with existing terminology makes sense. Not sure about the covered-by as it could mean implementation coverage or documentation coverage as well. Which reminds me that the L3 attributes tested, documented and implemented should be probably obsoleted by the new link attribute as well.

So I would rather propose something like tested, documented and implemented to make it completely unique/clear. What do you think?

Regarding the parent-of and child-of relations: Why do you think it would be better? Wherever possible I'd suggest to use as short names as possible. Here I'm not sure about the benefit.

One more general question: Is it ok that duplicate is bidirectional? Or should it be also duplicates and duplicated-by?

psss commented 3 years ago

Finally, to finish my brainstorm, what about heading towards a consistent bi-directional naming for all relations except for relates itself?

verifies - verified-by
implements - implemented-by
documents - documented-by
duplicates - duplicated-by
blocks - blocked-by
parent - parent-of
child - child-of

I'd say, having multiple names for the same relation (e.g. depends-on == blocked-by or verified-by == tested-by) is acceptable as long as the meaning is clear.

The default relation is relates.

psss commented 3 years ago

Time to complete the specification! I suggest to finalize it on tomorrow's tmt hacking session. To sum up current status: Storing the link relation directly in the key is the preferred way. Regarding the relation naming, I'd go with @WOnder93's proposal slightly extended to cover both directions in a consistent way and allowing aliases. If you have any feedback and cannot attend tomorrow's session, share your thoughts here. Thanks.

psss commented 3 years ago

The spec has been updated, rebased on the latest master and ready for the final review.

pkis commented 3 years ago

Hey, this will be a big step forward. The suggested relation types seems to be rich enough. They will help also defining relations between stories (like, story1 is child of sotry2).

psss commented 3 years ago

Thanks to all for your feedback and review. This was rather a long discussion but the elegant and concise format which came from it confirms it was worth it. Merged into master.

teemtee / tmt

Add 'link' as a core attribute #215

Relation in the key

Relation in the type