-
Notifications
You must be signed in to change notification settings - Fork 61
Make issues first class entities (#187) #188
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
Make issues first class entities (#187) #188
Conversation
external issue management software. This event provides provenance for an issue, | ||
and acts as a "handle" for future issue related events similarly to | ||
[EiffelActivityTriggeredEvent](./EiffelActivityTriggeredEvent.md) and | ||
[EiffelTestCaseTriggeredEvent](./EiffelTestCaseTriggeredEvent.md). It provides |
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 don't think it is fair to compare ID with ActT and TCT. The latter events represent that an operation is put in queue to be executed. I would propose you end the sentence after 'issue related events'. It is semantically quite similar to ED and FCD though.
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.
The description could definitely use some clarification. ID is certainly
similar semantically to FCD/ED; an issue really is just a specific kind of
flow context with additional data attached. What motivated the comparison to
ActT/TCT was the inclusion of state intrinsic to the entity. If we think
about an issue being defined, work being done on an issue, and being
resolved, it's quite similar to ActT, ActS, and ActF. We could say that ID
puts an operation (resolve this) on a queue (backlog) to be executed (by a
human). However, the similarity to ActT/TCT is moot without events like ISM.
### data.type | ||
__Type:__ String | ||
__Required:__ Yes | ||
__Legal values:__ BUG, IMPROVEMENT, FEATURE, WORK_ITEM, REQUIREMENT, OTHER |
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 this really be mandatory? And what if the type is changed in the issue management system, how should that be handled? I'd propose 'initialType' or 'initialCategory' and make it free text. It's values can be enforced by implementation specific guidelines instead.
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.
Eh, I just saw that we have called this 'type' in the current issue references and the legal values come from there. It's not your invention. Sorry :) Anyway, when we're now at it I still think this should not be mandatory and its values should be free text.
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.
The type changing in the issue management system is a good point. Similar to ISM, it's an instance of the question "Should Eiffel care about modifications to an entity's data?" which in turn depends on "To what degree does Eiffel care about describing an entity, and when does Eiffel simply identify it?" The latter question is answered in other events, for example ED and SCC, but has yet to be decided for issue related events.
This also relates to the earlier discussion about the semantic similarities of ID to FCD/ED vs ActT/TCT. The closer towards the identification side ID goes, the more similar to FCD, the closer to the descriptive side the closer to ActT.
As far as the values of data.type
being an enum or free form string, I assume that when you say "implementation specific guidelines" you're talking about using the vocabulary of the issue tracker as specified in data.tracker
. I'm concerned that removing the structure here leaves a lot of utility on the table. While subtle, making this free form has a significant consequence for consumers: they need to be aware of every issue type of every issue management software they want to consume. At this point, there have been N producer plugins in production emitting events from those issue trackers into Eiffel, and now M consumers need to speak the language of each of those N issue trackers anyways, so we've arrived back at the N*M number of integrations problem. If our goal is to enable heterogeneous systems to work together I think that requires having a common vocabulary with defined semantics.
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 agree that an Eiffel deployment needs to have a common vocabulary. I'm just not sure all of it needs to be defined in the protocol itself. In some parts it can be more convenient to define the legal values in guidelines/wrappers to be used within the company/deployment. I completely agree that we don't want different values on data.type from different issue trackers within the same company/deployment.
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.
Couldn't that be said about basically anything, though? I'd like some unambiguous rules as to what gets to be defined in the protocol, and what is left to guidelines/wrappers in each deployment. Right now it's sort of a gradient where we end up at slightly different places along the axis with every judgment call we make.
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.
Agreed. And that's a topic to discuss in another issue.
About this one, let's keep the type values there for now, since they are already part of the current events (IV and SCC). And that field is mandatory in those events too, so let's keep it mandatory for now.
to be a detailed description, as such information should be accessible by | ||
following __data.uri__. | ||
|
||
### data.initialCategory |
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.
See my comment on this field in the ISM event.
Furthermore, the description talks about 'state' but this field is called 'category'. I guess I would prefer 'initialState' here. 'category' is what I would use for the issue type instead...
Even further, do we really need this field when we have the initialStatus field below? Wouldn't initialCategory/initialState always be 'NEW' or possibly 'OPEN'? It couldn't be RESOLVED, could it? It comes down to the semantics of this event I guess. Should it be consumed as 'Hey, there is a new issue created, take a look!' or 'Yes, that is the existing issue I want to link to, give me its current details'
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.
The naming of these fields is a little unwieldly. The idea behind the category/status divide on ISM was to have a closed set of "something that needs work", "something being worked on", and "something that doesn't need any more work" in category
, and leave any specific subclasses of those up to status
. This would give consumers a simple and well defined path to react to/filter on the high level changes in category
without having to worry about the underlying issue management software but still allow the flexibility of having a custom status. "TODO", "DOING", and "DONE" would probably have been better choices for categories, but we didn't want to insinuate that we were forcing kanban style/terminology. The semantics of category
relied a bit on the context of being inside of an IssueStatusModifiedEvent, so initalCategory
sounds a bit funny in an ID. We were thinking of ID/ISM being closer to ActT/ActS, where an issue defined with a status/category would be emitted in Eiffel as ID immediately followed by ISM.
As for how the event should be consumed, while we should definitely keep consumers' needs in mind, I would be cautious of being perscriptive.
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.
Since we're moving to get the refactoring of SCC/IV into ID done first, I've removed initalCategory/initalStatus as they don't really make much sense without ISM existing.
__Required:__ Yes | ||
__Description:__ The identity of the issue. This is tracker dependent - most trackers have their own issue naming schemes. | ||
## Links | ||
### SUCCESFUL_ISSUE |
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 be 'SUCCESSFUL_ISSUE'
__Required:__ No | ||
__Legal targets:__ [EiffelIssueDefinedEvent](../eiffel-vocabulary/EiffelIssueDefinedEvent.md) | ||
__Multiple allowed:__ No | ||
__Description:__ Identifies an issue that has been succesfully verified. |
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 be 'successfully'
I've removed ISM/IA from the PR. What remains is essentially pulling the fields from |
{ | ||
"type": "ENVIRONMENT", | ||
"target": "aaaaaaaa-bbbb-5ccc-8ddd-eeeeeeeeeee4" | ||
}, |
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.
- meta.version 2.0.0
- The data.issues object should removed here.
"version": { | ||
"type": "string", | ||
"enum": [ "1.1.1" ], | ||
"default": "1.1.1" |
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.
2.0.0
"tracker": "GitHub", | ||
"details": "https://github.com/johndoe/myPrivateRepo/commit/fd090b60a4aedc5161da9c035a49b14a319829b4", | ||
"id": "42" | ||
}, |
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.
meta.version 2.0.0
"version": { | ||
"type": "string", | ||
"enum": [ "1.1.1" ], | ||
"default": "1.1.1" |
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.
2.0.0
👍 |
Making good progress! Will you propose the other events (EiffelIssueStatusModifiedEvent and EiffelIssueAssignedEvent) in this PR, too? |
I'd prefer that ISM and IA would come in a later PR. It it not as obvious to me that we want those events in the protocol. I would need to be convinced of the need of those additional events before I'd be happy to propose them being added. |
I don't see any more comments coming in, so let's move forward on this. Let's merge the new ID event and hold on other Issue events until we sort out #190 (anyone want to take a stab at this? - would be greatly appreciated, I'm rather strapped for time right now). We need a link from README.md to ID. Other than that, we should be good to go? If you have any objections, speak now and so on... |
Ping @jaden-young. Would you be willing to add the README link so I can merge, or would you rather that I added it? |
My apologies, the link has been added to the README. |
Applicable Issues
Issue #187
Description of the Change
Description of change follows from the initial description found in issue #187 prior to any feedback from the Eiffel community.
Alternate Designs
A possible alternative to introducing new issue related events would be to use the existing EiffelFlowContextDefinedEvent. We decided to introduce new event types because we felt the information conveyable by a FlowContext wasn't sufficient. A
uri
would be necessary to uniquely identify an issue out side of Eiffel, and attaching other data such astracker
is very useful. Coercing these into a FlowContext doesn't fit.Benefits
Same benefits as listed in the first comment of issue #187 prior to any feedback.
Possible Drawbacks
Same possible drawbacks as listed in the first comment of issue #187 prior to any feedback.
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: Jaden Young jaden.young@ndsu.edu