feat: add event_status to inline docs add ADR to support it #24
Conversation
mariajgrimaldi
force-pushed
the
MJG/event_status
branch
from
dcbe3d7
to
921ab18
Compare
mariajgrimaldi
force-pushed
the
MJG/event_status
branch
from
921ab18
to
cf5f8de
Compare
mariajgrimaldi
changed the title
mariajgrimaldi
changed the title
|
|
||
| Each Open edX Event will follow the following lifecycle: | ||
|
|
||
| State 1. Provisional |
There was a problem hiding this comment.
Some initial thoughts:
- Would provisional status affect versioning? Would you be more willing to fix issues without bumping versions, or will you have strict versioning while in a provisional state?
- By what process and who decides when it becomes active?
- Could an event go from provisional to removed, if it never becomes active?
- Who would use a provisional event, and what do they need to know about making this choice?
- What will prevent us from having provisional events forever? Does it require a review deadline? Seems similar to v0 APIs, and I wonder if we still have issues around that in the platform.
There was a problem hiding this comment.
Hi @robrap,
Thanks a lot for the thoughtful questions.
Would provisional status affect versioning? Would you be more willing to fix issues without bumping versions, or will you have strict versioning while in a provisional state?
The way we have discussed it, we always need to have strict versioning in place so that plugin developers and core-platform developers only have to agree on the version of this library that is being used. Then both can go their separate ways with a common definition and write testeable code without the plugin requiring the whole edx-platform repo.
Now this does create a big disconnect, because as you pointed in the next question:
By what process and who decides when it becomes active?
The process by which an event becomes active is the actual usage in the platform. By having this event_status annotation we only wanted to signal it in a way that is easy to pick up by an automated process of documentation.
The scenario we are trying to avoid is a developer finding out the definition of an event in this library and using it, only to later never see the event happening because it was not yet implemented in the platform or it was deprecated or replaced or any of the above.
Could an event go from provisional to removed, if it never becomes active?
The wording seems a bit off for me even as non native speaker, but yes. We could change "provisional" to "draft" or "proposal" or something to that effect.
Who would use a provisional event, and what do they need to know about making this choice?
Provisional is a way to mark that the event is fully defined here, but might not be used in the edx-platform yet. A developer that wants to use a provisional event should go to the platform and see if it is in use. If so they should also make a PR here to change it to active.
What will prevent us from having provisional events forever? Does it require a review deadline? Seems similar to v0 APIs, and I wonder if we still have issues around that in the platform.
I think you are correctly pointing out here the eternal problem of keeping the annotations updated. We had a long discussion with @mariajgrimaldi about this. Maybe is even worse that we though because of the versioning process.
I'll ilustrate from the point of view of a developer creating a new event.
At openedx-events:
- Dev creates an event and leave it with provisional status.
- It gets merged
- A semver tag is made e.g. 1.1.0
At edx-platform:
- Dev updates the requirement of openedx-events to 1.1.0
- Dev uses the new event and the change lands in master. In theory this makes the event active, but my 1.1.0 version has it as provisional.
Now Dev needs to make this chores just to keep this updated:
- go to openedx-event and mark it active
- get it merged
- create new semver tag 1.1.1
- go to edx-platform and update to 1.1.1
- get it merged
This is a lot of work and in the meantime anyone with a version of edx-platform that already has the event in full action still gets the message that is provisional in the corresponding library and might not want to use it. The same dev could skip the last chores (not that they should, but it could happen) and leave a worse problem than the lack of information of the event_status would have caused.
I'm thinking now that a better way would be to keep this information closer to the usage of the events in the edx-platform repo instead of the definition in the openedx-events repo. I updated a previous PR with a version of the general hooks framework documentation that lives in edx-platform to reflect how this would look: https://github.com/edx/edx-platform/blob/5d7df3606822cd4e577ea6abeb386194e81e1201/docs/guides/hooks/index.rst.
There was a problem hiding this comment.
@felipemontoya: I see. I think this helps clarify a lot. Thank you.
- Do you think the terms
definedandimplementedmore precisely describe what you had meant byprovisionalandactive? That is what I am hearing in your above comments. - I agree that trying to document the events as
implementednext to the definition sounds painful to keep up-to-date and accurate, especially with the versioning issue you described. - It would be nice if documentation for
implementedevents could also be automated. This might come in the form of:
a. Automatically finding implementations in code.
b. Annotating implementations, and finding those annotations.
c. A runtime API endpoint that pseudo-documents based on what was loaded. Our toggles API endpoint is like this.
d. Other ideas? The alternative being a manual document that we try to keep up-to-date. - Ultimately, how we handle
implementeddocumentation may lead us in different directions for where we might want to mark events asdeprecated. Not sure.
There was a problem hiding this comment.
Hey @robrap, thanks for the feedback.
About # 2, we've created a sphinx extension in this PR that pulls the inline code annotations. In previous comments, you mentioned -here- docs generation from dependencies were not possible for now. From what I understand, I understand it's for the path used to generate the docs, am I right? Well, for now, I added a fixed path to generate Open edX Events documentation. Here are the modifications in the Sphinx configuration -for testing purposes-.
If we could use this, what would be the process of updating the documentation? Your insight would be beneficial.
There was a problem hiding this comment.
@mariajgrimaldi: There are a number of ways to go, and this will take some thinking through. I think you'll need to discuss as part of the community or with Ned and his team.
One idea would be to generate the event definition documentation as part of the openedx-events docs in this repo. This repo could publish its docs to Readthedocs, and edx-platform docs could provide a link (or include?) of this doc.
Additionally, Régis wrote the original Sphinx code for toggles and may have some thoughts and opinions.
Please take this comment as non-blocking food-for-thought that you can discuss with others that may be able to spend more time on this. Good luck, and I hope I was able to help get the ball rolling for you. Thank you.
| 2. Each event must go through each state in order. First, must be created | ||
| in this repository with the state `provisional`, when Open edX accepts it | ||
| must change to `active` and when the community decides to deprecate it, it | ||
| must be updated to `deprecated`, then `removed`. |
|
|
||
| 2. Each event must go through each state in order. First, must be created | ||
| in this repository with the state `provisional`, when Open edX accepts it | ||
| must change to `active` and when the community decides to deprecate it, it |
There was a problem hiding this comment.
Or removed if it isn’t accepted?
| State 3. Deprecated | ||
| ~~~~~~~~~~~~~~~~~~~ | ||
|
|
||
| Events that members of the community decide to deprecate in Open edX platform. |
There was a problem hiding this comment.
Should we mention and link to the DEPR process? I imagine this won’t need a unique process.
| ~~~~~~~~~~~~~~~~~ | ||
|
|
||
| Events that members of the community removed from Open edX platform after | ||
| documentation and discussion of the removal. |
There was a problem hiding this comment.
Once we get to removal, wouldn’t it just be deleted along with its documentation? Would documentation for this (as well as other state changes) be captured in the changelog?
| must change to `active` and when the community decides to deprecate it, it | ||
| must be updated to `deprecated`, then `removed`. | ||
|
|
||
| 1. Each state must be up-to-date. |
There was a problem hiding this comment.
| 1. Each state must be up-to-date. | |
| 3. Each state must be up-to-date. |
- What does this mean exactly? Is it about moving the process forward, or keeping documentation updated, or multiple things?
|
|
||
| Each Open edX Event will evolve according to the needs of the community. | ||
| For that reason, with this ADR, we will define a lifecycle that each | ||
| event will follow individually. |
There was a problem hiding this comment.
Reminder that docs shouldn’t use hard line breaks.
|
what should we do with this PR? @felipemontoya |
|
Although it might be strange in a common library, I'm wondering if we could have an index doc here with links to all the docs like the edx-platform event index. Maybe each link could note the domains that are implemented, so if you want to learn about a particular event, you could see its status without looking too far. |
Description:
This PR adds an ADR that supports the event_status as part of the inline code documentation of Open edX Events definitions. It will determine which events are brand new, already used by the community, replaced, or removed.