[BD-21] Simplify feature toggle annotation configuration #49

regisb · 2020-09-02T08:24:57Z

Description: Simplify the feature toggle configuration following the conversation with @robrap and @nasthagiri https://github.com/edx/edx-platform/pull/24815#issuecomment-681174018

JIRA: https://openedx.atlassian.net/wiki/spaces/COMM/pages/1596358943

Dependencies: A corresponding PR on OEP-17 will have to be made.

Merge deadline: None.

Reviewers:

Merge checklist:

Post merge:

Create a tag
Check new version is pushed to PyPi after tag-triggered build is
finished.
Delete working branch (if not needed anymore)

Author concerns: What should we do with toggle_status? I took the liberty of switching it to a choice field, but should we keep this field at all?

openedx-webhooks · 2020-09-02T08:25:02Z

Thanks for the pull request, @regisb! I've created BLENDED-573 to keep track of it in Jira. (The original issue OSPR-4936 has been deleted.) More details are on the BD-21 project page.

When this pull request is ready, tag your edX technical lead.

regisb · 2020-09-02T08:29:43Z

~~The version number has not been updated, yet, but~~ this is ready for review.

EDIT 2020/09/07: version number has been updated

robrap

Thanks @regisb. I added questions/comments for you and @nasthagiri.

code_annotations/config_and_tools/config/feature_toggle_annotations.yaml

robrap · 2020-09-02T21:42:49Z

code_annotations/config_and_tools/config/feature_toggle_annotations.yaml

    - ".. toggle_use_cases:":
-        choices: [incremental_release, launch_date, monitored_rollout, graceful_degradation, beta_testing, vip, opt_out, opt_in, open_edx]
+        choices: [temporary, circuit_breaker, vip, opt_out, opt_in, open_edx]
    - ".. toggle_creation_date:":
    - ".. toggle_expiration_date:":
    - ".. toggle_warnings:":
    - ".. toggle_tickets:":


@nasthagiri: Do we want this to be about removal (e.g. toggle_removal_tickets), or do we want this to any related tickets, which might be about removal, but may also be about the background of the ticket?

What if we just eliminate this field altogether?

Given that we'll soon have this report and can hold people accountable via the report, we don't need them to additionally manage the removal via tickets as well. It's up to them if they still choose to use tickets as an additional mechanism. But we no longer need to prescribe it.

If there's any additional context they want to provide about a toggle, they can still provide a link to a ticket/etc in the free-form description.

I think there is a misunderstanding: until now, I have used this field to document the ticket or Github PR that justifies the creation of the feature toggle, not its removal. This is useful information that helps put the feature toggle back in context (IMHO).

It could be used either way. The question is whether it is important enough to have its own annotation, rather than being a part of the description like is done for commit comments.

A separate, but related topic, when we write up the how_to, is there any of this metadata that we think would be useful as part of a convention for naming the actual toggles, or do we think these annotations remove the need for that?

If we keep this field, based on how @regisb is describing his current usage, it would be good to rename it. As of now, its name is not self-describing. Also, I am concerned about encouraging JIRA tickets since not all JIRA projects are public - and almost all require JIRA login.

Rename ideas:

toggle_description_reference

toggle_creation_reference

toggle_creation_pr

I think Description Reference and Creation Reference are a bit too vague, and people might as well include whatever relevant links they want in the description.

Creation PR is more specific, but couldn’t git blame answer this in the rare case it is needed and difficult to get from the git link?

I know the JIRA tickets aren’t always public, but I am fine with people including them in the description as they do on PRs whether or not they are public, as added context.

I personally would drop this (with 75%) confidence, but if either of you feel it is important to keep/rename, I think that would be fine.

This brought up a related question for me. If someone does want to include links in the description, what format do they use? If we want people to use rst format, does our code-annotation parsing support it? Our how_to should mention what Rst is supported that won’t interfere with our parsing.

I see what you mean: any relevant information present in a JIRA ticket should actually be included in the feature toggle description. And many JIRA tickets are unavailable to non-edX contributors. On the other hand, this field can refer to a Github PR, as I have done in edx-platform; but there is a small chicken-and-egg problem here, as the PR does not exist at the time when we create the annotation.

Ideally, if the annotation author feels like they should include reference links, then they could add them to the description. The problem is that, as it stands, annotations are assumed to be raw text. For instance, the following formatting elements will not work as expected in an annotation field: clickable links ( `... <...>`__), reference to other elements (:ref:`... <...>`_), verbatim code (``def func():...``), bold/italic formatting (**text**/*text*). All these elements would be useful in annotation fields, of course, but at the moment they are not available to us.

We could add such formatting to the annotation format -- but it would require some additional work.

I suggest we keep this toggle_ticket field until we figure out a way to properly format annotation fields. Then we'll be able to move the links to the toggle description field.

I say we merge and come back to this if we want to remove or rename.

code_annotations/config_and_tools/config/feature_toggle_annotations.yaml

As far as we understand, this implementation is used nowhere.

regisb · 2020-09-07T06:56:18Z

@robrap I have updated this PR following @nasthagiri's and your own recommendations.

robrap · 2020-09-07T12:04:18Z

code_annotations/config_and_tools/config/feature_toggle_annotations.yaml

    - ".. toggle_use_cases:":
-        choices: [incremental_release, launch_date, monitored_rollout, graceful_degradation, beta_testing, vip, opt_out, opt_in, open_edx]
+        choices: [temporary, circuit_breaker, vip, opt_out, opt_in, open_edx]
    - ".. toggle_creation_date:":
    - ".. toggle_expiration_date:":
    - ".. toggle_warnings:":
    - ".. toggle_tickets:":


It could be used either way. The question is whether it is important enough to have its own annotation, rather than being a part of the description like is done for commit comments.

A separate, but related topic, when we write up the how_to, is there any of this metadata that we think would be useful as part of a convention for naming the actual toggles, or do we think these annotations remove the need for that?

code_annotations/config_and_tools/config/feature_toggle_annotations.yaml

robrap · 2020-09-07T12:14:05Z

@regisb: We have a holiday today. I’ll get back to this tomorrow. None of my more recent comments are blockers to the changes already made, so we could merge and iterate. However, could you rename the expiration date as Nimisha proposed to match the DEPR field.

The following changes are made to the feature toggle annotation config: 1. the `toggle_category` annotation is deprecated 2. the list of `toggle_use_cases` is simplified 3. the `toggle_status` field is deprecated 4. `toggle_expiration_date` was renamed to `toggle_target_removal_date` 5. Add SettingToggle, SettingDictToggle toggle implementations OEP-17 will be updated correspondingly.

regisb · 2020-09-08T07:17:37Z

Thanks for your review on a holiday @robrap!

could you rename the expiration date as Nimisha proposed to match the DEPR field.

I believe I have already made this change: toggle_expiration_date was renamed to toggle_target_removal_date. Did you have something else in mind?

robrap

Let me know when this is squashed and ready to merge.

robrap · 2020-09-09T01:43:32Z

code_annotations/config_and_tools/config/feature_toggle_annotations.yaml

    - ".. toggle_use_cases:":
-        choices: [incremental_release, launch_date, monitored_rollout, graceful_degradation, beta_testing, vip, opt_out, opt_in, open_edx]
+        choices: [temporary, circuit_breaker, vip, opt_out, opt_in, open_edx]
    - ".. toggle_creation_date:":
    - ".. toggle_expiration_date:":
    - ".. toggle_warnings:":
    - ".. toggle_tickets:":


I say we merge and come back to this if we want to remove or rename.

code_annotations/config_and_tools/config/feature_toggle_annotations.yaml

openedx-webhooks added needs triage open-source-contribution PR author is not from Axim or 2U labels Sep 2, 2020

regisb mentioned this pull request Sep 2, 2020

[BD-21] Add a "How To document feature toggles" documentation page openedx/edx-toggles#61

Merged

10 tasks

regisb force-pushed the regisb/update-featuretoggle-config branch from b8b0a41 to a792279 Compare September 2, 2020 08:29

natabene changed the title ~~[BD-21 Simplify feature toggle annotation configuration~~ [BD-21] Simplify feature toggle annotation configuration Sep 2, 2020

openedx-webhooks added blended PR is managed through 2U's blended developmnt program and removed open-source-contribution PR author is not from Axim or 2U labels Sep 2, 2020

robrap reviewed Sep 2, 2020

View reviewed changes

nasthagiri reviewed Sep 4, 2020

View reviewed changes

code_annotations/config_and_tools/config/feature_toggle_annotations.yaml Show resolved Hide resolved

Deprecate WaffleSample feature toggle annotation implementation

eba1ecd

As far as we understand, this implementation is used nowhere.

regisb force-pushed the regisb/update-featuretoggle-config branch 2 times, most recently from 2f785e3 to 8d2df85 Compare September 7, 2020 06:52

regisb mentioned this pull request Sep 7, 2020

[BD-21] Document feature toggles openedx/edx-platform#24815

Merged

robrap reviewed Sep 7, 2020

View reviewed changes

regisb force-pushed the regisb/update-featuretoggle-config branch from 8d2df85 to fa1f5b0 Compare September 8, 2020 07:14

robrap approved these changes Sep 9, 2020

View reviewed changes

robrap merged commit e82dbf0 into openedx:master Sep 10, 2020

regisb deleted the regisb/update-featuretoggle-config branch September 10, 2020 16:00

openedx-webhooks added merged and removed needs triage labels Sep 10, 2020

regisb mentioned this pull request Nov 6, 2020

Adding incremental_release as another option for toggle_use_case #53

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[BD-21] Simplify feature toggle annotation configuration #49

[BD-21] Simplify feature toggle annotation configuration #49

regisb commented Sep 2, 2020 •

edited

Loading

openedx-webhooks commented Sep 2, 2020 •

edited

Loading

regisb commented Sep 2, 2020 •

edited

Loading

robrap left a comment

robrap Sep 2, 2020

nasthagiri Sep 4, 2020

regisb Sep 7, 2020

robrap Sep 7, 2020

nasthagiri Sep 7, 2020

robrap Sep 7, 2020

regisb Sep 8, 2020

robrap Sep 9, 2020

regisb commented Sep 7, 2020

robrap Sep 7, 2020

robrap commented Sep 7, 2020

regisb commented Sep 8, 2020

robrap left a comment

robrap Sep 9, 2020

[BD-21] Simplify feature toggle annotation configuration #49

[BD-21] Simplify feature toggle annotation configuration #49

Conversation

regisb commented Sep 2, 2020 • edited Loading

openedx-webhooks commented Sep 2, 2020 • edited Loading

regisb commented Sep 2, 2020 • edited Loading

robrap left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

regisb commented Sep 7, 2020

Choose a reason for hiding this comment

robrap commented Sep 7, 2020

regisb commented Sep 8, 2020

robrap left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

regisb commented Sep 2, 2020 •

edited

Loading

openedx-webhooks commented Sep 2, 2020 •

edited

Loading

regisb commented Sep 2, 2020 •

edited

Loading