Skip to content

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

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.

Sign up for GitHub

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

Merged
robrap merged 2 commits into from
Sep 10, 2020
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
eba1ecd
Deprecate WaffleSample feature toggle annotation implementation
regisb Sep 1, 2020
fa1f5b0
[BD-21] Update feature toggle annotation config
regisb Sep 2, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions CHANGELOG.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,11 @@ Change Log

.. There should always be an "Unreleased" section for changes pending release.

[0.7.0] - 2020-09-07
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

* Simplify the feature toggle annotation format

[0.6.0] - 2020-08-27
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Expand Down
2 changes: 1 addition & 1 deletion code_annotations/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,4 +2,4 @@
Extensible tools for parsing annotations in codebases.
"""

__version__ = '0.6.0'
__version__ = '0.7.0'
8 changes: 3 additions & 5 deletions code_annotations/config_and_tools/config/feature_toggle_annotations.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,17 +11,15 @@ annotations:
feature_toggle:
- ".. toggle_name:":
- ".. toggle_implementation:":
choices: [ExperimentWaffleFlag, WaffleFlag, WaffleSample, WaffleSwitch, CourseWaffleFlag, ConfigurationModel, DjangoSetting]
choices: [WaffleFlag, WaffleSwitch, CourseWaffleFlag, ExperimentWaffleFlag, SettingToggle, SettingDictToggle, ConfigurationModel, DjangoSetting]
- ".. toggle_default:":
- ".. toggle_description:":
- ".. toggle_category:":
- ".. 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_target_removal_date:":
- ".. toggle_warnings:":
- ".. toggle_tickets:":
Copy link
Contributor

@robrap robrap Sep 2, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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?

Copy link

@nasthagiri nasthagiri Sep 4, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@regisb regisb Sep 7, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Contributor

@robrap robrap Sep 7, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link

@nasthagiri nasthagiri Sep 7, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@robrap robrap Sep 7, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@regisb regisb Sep 8, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@robrap robrap Sep 9, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

- ".. toggle_status:":
extensions:
python:
- py
Expand Down