Discussion regarding adding pull requests to edx-toggle annotations

Development
#1

Hello everyone :wave:

There was a short discussion started by @regis on a pull request regarding one of the annotated toggle tickets TNL-5041 being inaccessible by non-edX people.

During the discussion @robrap proposed having the toggle annotation link point to a pull request instead, to which an edX member would copy any relevant information.

According to one of @robrap’s comments, the toggle annotation was planned for calling out a DEPR ticket, yet isn’t being used for that often.

Thus, we ended up with multiple different suggestions, by both @robrap and @nimisha.

I’ll attempt to voice their suggestions through a series of questions mentioned below:

  • Is it useful to have a separate annotation for pull requests or not? ~ reference
    • If so…
      • What should the separate annotation be called?
    • If not…
      • Should we just document things to consider including in the description?
  • Should the same rules be applied to the settings annotations? ~ reference
  • Should ADRs be created instead of linking pull requests? ~ reference

I was going to consider building a poll, but I’ll give a chance for members to express their opinions more freely in case anyone else had any suggestion that wasn’t mentioned earlier.

#2

Here are my 2 cents:

  1. Linking to a separate Jira ticket or PR should not make up for an extensive description in the toggle_description field.
  2. Private Jira tickets should never be mentioned in toggle_ticket because they are inaccessible by most Open edX developers…
  3. … but neither should PRs be linked to, because this is too “meta”: usually, the PR that introduces the toggle annotation will be the same as the one referenced in the toggle_ticket annotation.
  4. … and neither should we expect to create a new ADR for every new toggle. This would be too time-consuming and would dilute the value of ADRs.

Thus: if we cannot make Jira tickets public, the only solution is to provide better ticket descriptions.

#3

Agreed. So, it seems reasonable to have a separate annotation.

Many of edX’s tickets are private, so if we agree on this, asking for a ticket (i.e. toggle_tickets) is probably not going to lead to what we want.

I’m not sure how I feel about this. At this point we have a lot of legacy toggles that are being annotated after the fact. However, one could claim that as long as the PR that adds the annotations provides reference links to other PRs, it doesn’t necessarily have to go in the annotations. I just want to save others from doing the same hunt as the annotation author had to do to gather data.

Totally agree. However, there have been a bunch of cases where new rollout toggles were mentioned in an ADR, and I think these ADRs should be candidates.

I’m not clear on what “…better ticket descriptions” means? Do you mean better toggle_description or other?

I was going to propose that we replace toggle_tickets with toggle_supporting_links (or something like that), and have the documentation for annotating toggles note the following possibilities:

  • Links to relevant supporting documentation, like how-tos, ADRs, etc.
  • Link to a DEPR ticket (or DEPR discussion forum link?)

I was going to include PRs and public JIRA tickets, but maybe we could instead say that links to PRs or other JIRA tickets probably better belong in the “Supporting information” section of the description of the PR which is adding the annotations.

#4

I vote (probably) yes. Assuming we keep the annotation for toggles and use it for more than DEPR, I don’t see why it would be any less relevant for settings.

#5

Yes, my bad, exactly!

My opinion is that if we can’t access Jira tickets, then the information that they contain must be reported in the toggle descriptions, which is the only place where they truly belong: it’s close to the code, under version control, readable and writable by anyone.

Now of course if there are publicly accessible documents, such as ADRs or tickets, that are related to the toggle/setting then of course they should be linked to in an annotation.

#6

I’m going to sound a little bit self-contradictory here, but I think this is going to introduce some difficulty, as time passes.

If changes are done to the toggle or setting in a new pull request, then we’d end up in a similar situation to this pull request, where I ended up linking multiple pull requests for some of the toggles.

This might also end up happening if there’s links to a specific ticket (unless only new tickets are referenced, and the tickets contain the necessary context about the previous tickets).

Therefore, I find myself leaning a little bit towards @regis’s suggestion, to provide more details in the description, instead.

There are, also, a lot of pull requests by edX developers which don’t have any descriptions or enough details about any toggle or setting. So usually, we’d mostly end up relying on reading the diff.

Maybe instead of linking the pull requests in the annotation, the pull request introducing/changing the annotation would contain a reference to any related pull request used by the author?

#7

@nizar @regis: How does this sound?

Annotations:

  • toggle_supporting_docs (renamed from toggle_tickets)
  • setting_supporting_docs

Allowed links:

  • How-Tos, ADRs, or other more permanent documentation.
  • DEPR tickets

Unallowed links:

  • PRs
  • (Non-DEPR) JIRA tickets.

Unallowed links may still be useful background for the annotation, and should be included in the PR description of the PR that adds the annotation itself.

Additional Notes:

  • It is understood that renaming an annotation would require some combo of expand/contract or lint amnesty.
  • We may want to enforce with linting.
  • I would suggest that this annotation be included in the sphinx doc output.
1 Like
#8

I like the suggestion, and the clarification that unallowed links can be included in the toggle description.

I think @regis’s opinion will be much more valuable than mine regarding the additional notes, since I’ve only been participating in the documentation of the toggles.

#9

@nizar: I edited my comment to make it more clear. I was not saying to add unallowed links in the toggle description, but that they could be added to the PR description of the PR that is adding the toggle annotation.

1 Like
#10

That sounds better than accumulating some links in the annotations, thanks for clarifying that :+1:

#11

I think it’s a good proposal. My only issue is with the naming: “supporting docs” does not make it explicit that only links would be supported for this annotation token. I would suggest simply toggle_links/setting_links.

#12

Regarding the name, what about toggle_doc_links?

Also, after reviewing a bunch of annotations, I’m wondering if toggle_removal_tickets (or toggle_removal_ticket_links or toggle_removal_links) should be a separate annotation dedicated to tickets supporting the removal of a temporary toggle. This annotation could be used for DEPR, but also for private or public tickets created to track removing a temporary toggle.

Basically, if a team wants to create a temporary toggle and shortly after remove it, and they want a way to track that, I want to create as little friction as possible to that toggle being yanked.

Thoughts?

1 Like
#13

:ok_hand:

Do you have examples of toggles in mind? I think we should try to keep the list of annotation tokens as short as possible, and that such links may go to toggle_doc_links – but I don’t have a very strong opinion on that.

1 Like
Which toggle annotations should be published?
#14

Feel free to comment on the PR. If it lands before you get a chance, feel free to open a follow-up PR.

Thank you all for the conversation here. Your thoughts were a big help to me, because you can see I was all over the place with this.

1 Like