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
Add PRECURSOR link type and clarify activity linking #287
Add PRECURSOR link type and clarify activity linking #287
Conversation
3e7b23f
to
45091a9
Compare
adec02a
to
f008dfc
Compare
d7ea429
to
9faee45
Compare
First activity linking svg draft Update readme Update link descriptions
9faee45
to
9f0dc8a
Compare
README.md eiffel-syntax-and-usage/activity-linking.md eiffel-syntax-and-usage/glossary.md eiffel-syntax-and-usage/precursor-parallel.png eiffel-syntax-and-usage/precursor-simple.png eiffel-vocabulary/EiffelActivityTriggeredEvent.md tox.ini usage-examples/activity-linking/README.md usage-examples/activity-linking/activity-linking.svg usage-examples/activity-linking/combined-pipeline.md usage-examples/activity-linking/event-driven-pipeline.md usage-examples/activity-linking/network-of-pipelines.md usage-examples/activity-linking/orchestrated-pipeline.md
Added PRECURSOR to EiffelTestSuiteStartedEvent Update event graphs, and added Inkscape svg source files Resolved outstanding question left in the doc earlier Ready to be reviewed
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.
Some smaller suggestions
__Required:__ No | ||
__Legal targets:__ [EiffelTestSuiteStartedEvent](./EiffelTestSuiteStartedEvent.md) | ||
__Multiple allowed:__ Yes | ||
__Description:__ Used to declare temporal relationships between test suite activities in a pipeline step, i.e. what other test suite (or or test suites) preceded this activity. This link type applies primarily to non event-triggered test suite executions. |
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.
Any reason why this is not the same text as in ActT? E.g. it doesn't contain links.
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.
Yes. If I would link to activity it could confuse the reader that this is a sub-type or something of an activity event type. The same for the pipeline link, as this text states pipeline step and not just pipeline. Do you have a good suggestion on what links to put in there?
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.
Could the activity linking page be made more generic so that we can link to it in good faith? Pretty much everything on that page applies to test suites as well as activities so maybe we should be more explicit about that somewhere? Some kind of "where we say activity we actually mean activity or test suite" disclaimer at the top.
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.
Not sure. When looking at this I also see that we currently mention TestCase* events as being some kind of activity events: https://github.com/eiffel-community/eiffel/blob/master/eiffel-syntax-and-usage/glossary.md#activity. I don't have a good way to describe in what way test suites are considered activities, but test cases are not, and thus why PRECURSOR could link between TSS:es and not TCT:s. See also Mattias comment on the TCT event type.
Maybe I should include some further clarification on that glossary page within this PR as well?
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've updated the PR now to also allow PRECURSOR links between TCT events, and thus rewritten some parts of the PR that relate to that update.
|
||
This link type is relevant mostly to non event-triggered activities. It is though recommended to also use it for event-triggered activities, as it helps to visualize the full chain of activities in a pipeline in a common way regardless of how each activity was triggered. By always providing PRECURSOR links between activities, a visualization tool does not need to follow any CAUSE links in order to visualize the activity relationships. | ||
|
||
For event links between serially executed pipelines, e.g. when a source change in an integration repository is automatically created by an update to an upstream dependency for that integration, there would be a CAUSE link to the event notifying that updated dependency. The protocol currently does not recommend to use a PRECURSOR link in that scenario. |
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 think this need some more explanation or maybe a picture.
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'll rephrase it slightly. Don't have a good idea on a picture for it. If there is a better suggestion for improved text, or a picture, I'm open for it :)
Co-authored-by: Mattias Linnér <54929896+m-linner-ericsson@users.noreply.github.com>
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.
LGTM on a high level, just a few buglets and nits.
|
||
This link type is relevant mostly to non event-triggered activities. It is though recommended to also use it for event-triggered activities, as it helps visualizing the full chain of activities in a pipeline in a common way regardless of how each activity was triggered. By always providing PRECURSOR links between activities, a visualization tool does not need to follow any CAUSE links in order to visualize the activity relationships. | ||
|
||
For event links between two or more complete pipelines, e.g. when a source change in an integration repository is automatically created by an update to an upstream dependency for that integration, there would be a CAUSE link to the event notifying that updated dependency. The protocol currently does not recommend to use a PRECURSOR link in that scenario. |
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.
What's an integration repository?
This paragraph isn't terribly clear to me in general. The preceding paragraph recommends using PRECURSOR together with CAUSE but in this paragraph we suddenly don't recommend using PRECURSOR with CAUSE.
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've tried to clarify this in the commit I'll push soon.
__Required:__ No | ||
__Legal targets:__ [EiffelTestSuiteStartedEvent](./EiffelTestSuiteStartedEvent.md) | ||
__Multiple allowed:__ Yes | ||
__Description:__ Used to declare temporal relationships between test suite activities in a pipeline step, i.e. what other test suite (or or test suites) preceded this activity. This link type applies primarily to non event-triggered test suite executions. |
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.
Could the activity linking page be made more generic so that we can link to it in good faith? Pretty much everything on that page applies to test suites as well as activities so maybe we should be more explicit about that somewhere? Some kind of "where we say activity we actually mean activity or test suite" disclaimer at the top.
__Legal sources:__ [EiffelActivityTriggeredEvent](../eiffel-vocabulary/EiffelActivityTriggeredEvent.md), | ||
[EiffelTestSuiteStartedEvent](../eiffel-vocabulary/EiffelTestSuiteStartedEvent.md) | ||
__Legal targets:__ [EiffelActivityTriggeredEvent](../eiffel-vocabulary/EiffelActivityTriggeredEvent.md), | ||
[EiffelTestSuiteStartedEvent](../eiffel-vocabulary/EiffelTestSuiteStartedEvent.md) |
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.
Should we also allow a link between TCT? I guess you would like to order tests also?
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.
Yes, in fact I believe we should. I'll update that here and also on some other places where these relationships are exemplified.
I've done some decent effort updates to the PR now, based on all comments given so far. Please re-review. |
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.
Just a couple of typographic nits.
Applicable Issues
Fixes #227
Description of the Change
This PR adds a new link type to be used between activity events - PRECURSOR. Main purpose of that link type is to provide improved traceability between activities within a pipeline.
Together with this we also extend and clarify some terms in the glossary.
Alternate Designs
An alternative would be to use CAUSE links to provide traceability between activities, which has a number of drawbacks. For example:
Benefits
Possible Drawbacks
Sign-off
Developer's Certificate of Origin 1.1
By making a contribution to this project, I certify that:
(a) The contribution was created in whole or in part by me and I
have the right to submit it under the open source license
indicated in the file; or
(b) The contribution is based upon previous work that, to the best
of my knowledge, is covered under an appropriate open source
license and I have the right under that license to submit that
work with modifications, whether created in whole or in part
by me, under the same open source license (unless I am
permitted to submit under a different license), as indicated
in the file; or
(c) The contribution was provided directly to me by some other
person who certified (a), (b) or (c) and I have not modified
it.
(d) I understand and agree that this project and the contribution
are public and that a record of the contribution (including all
personal information I submit with it, including my sign-off) is
maintained indefinitely and may be redistributed consistent with
this project or the open source license(s) involved.
Signed-off-by: Emil Bäckmark, emil.backmark@ericsson.com