[BD-21] Generate feature toggle documentation

[regisb]

We introduce a new documentation target, where we use the feature toggle
annotation report (a yaml file) to generate html documentation. To do
so, we develop a sphinx plugin that introduces a .. featuretoggles::
directive. This directive parses the yml report file and generates
the docs.

[openedx-webhooks]

Thanks for the pull request, @regisb! I've created BLENDED-478 to keep track of it in Jira. More details are on the BD-21 project page.

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

[regisb]

A preview of the generated docs is available here as a compressed zip file: https://www.dropbox.com/s/qv0rk3aaqps1fbg/featuretoggles-docs.zip?dl=0

[natabene]

@regisb Thank you for your contribution. Is this ready for our review?

[regisb]

@natabene no, not yet. We are still discussing the approach.

[robrap]

Assuming we only plan on having the docs available in readthedocs, and not as RST files available in the repo, I personally think this is fine and don't have a lot to say about introducing the new directive. Seems comprehensible enough.

I think others will make sense for reviewing the sphinx setup, etc. I'm not clear if make docs is what is used to generate everything that is to be deployed together in readthedocs?

[robrap]

I know it needs more discussion, but why couldn't this stay here and get versioned with the code?

[regisb]

I get it could. I don't have a very strong opinion on this. Generally speaking, I avoid versioning of automatically-generated assets. If we version this file, then developers will have to remember to update it very often. For instance, it will become outdated as soon as the position of feature toggles in the source code will be modified, as this report includes line numbers. Plus, generating this file requires the installation of code_annotations as well as the feature_toggle_annotations.yaml configuration file from the edx-toggles repo, so I bet developers will frequently forget to update the yaml report.

[robrap]

I was imagining this being automated so it would be up to date, but that automation clearly doesn't exist yet.
For now, if your code generates and uses this file and it doesn't need to be committed, then feel free to remove it.

[robrap]

FYI: rebasing should resolve Python 3.8 failures.

[robrap]

Agreed. I imagined an automated job to keep the yml up to date.

[robrap]

@regisb: You'll need to cherry-pick this commit: edx@a73b923

[regisb]

To get this PR ready for review, I need one additional decision. Should we version in edx-platform:

1. The report.yml file generated by code_annotations static_find_annotations --config_file ../edx-toggles/feature_toggle_annotations.yaml --no_lint --report_path ./reports
2. The configuration file from edx-toggles used to generate the report.
3. Both
4. Neither

My vote goes to option 2: move the feature_toggle_annotations.yaml to the Sphinx extension folder in docs/featuretoggles/extensions. This configuration file will then be used by the plugin to generate the report: not by invoking code_annotations from the CLI, but by interfacing directly with the python code:

from code_annotations.base import AnnotationConfig
from code_annotations.find_static import StaticSearch
config = AnnotationConfig("./feature_toggle_annotations.yaml")
results = StaticSearch(config).search()

By not versioning report.yml we ensure that developers will not accidentally forget to generate it. The data will always be kept up-to-date. Later, we will most probably have to document feature toggles from other repositories. It will then be time to move the "featuretoggles" Sphinx extension to a dedicated repository, along with the "djangosettings" extension which we will soon create.

What do you think @robrap?

[robrap]

@regisb: We are already using feature_toggle_annotations.yaml in another job where we are reporting on, so I don't think that solution works.

1. This may be a dumb question, but is it not possible to make the edx-toggles a doc requirement and get access to feature_toggle_annotations.yaml?
2. If the only way turns out to be to have the file committed, we would automate that process. It would not be dependent on someone remembering/forgetting.

[regisb]

This may be a dumb question, but is it not possible to make the edx-toggles a doc requirement and get access to feature_toggle_annotations.yaml?

Yes you're right @robrap, this is probably the best solution. Especially since edx-toggles has very few base requirements. I would move the Sphinx extension from edx-platform to edx-toggles, and thus most of the changes from this PR there. However, edx-toggles is currently not available from pypi. Is there a process to publish this package there? Should I copy and adapt the .travis.yml file from code-annotations? Or should I simply add the github dependency under the form -e git+https://github.com/edx/edx-toggles.git?

[robrap]

@regisb: I'll add a ticket to get edx-toggles into PyPI and hopefully we'll get that done very soon.

[robrap]

@regisb:

1. Although I created a ticket for edx-toggles=>PyPI, you should use the github dependencies for now to unblock yourself.
2. For now, the annotations yml should be generated as-needed and not committed. Is that something you want to do here, or in a follow-up?

[regisb]

Although I created a ticket for edx-toggles=>PyPI, you should use the github dependencies for now to unblock yourself.

👌

For now, the annotations yml should be generated as-needed and not committed. Is that something you want to do here, or in a follow-up?

I already wrote the code. Since it will be part of the featuretoggles Sphinx extension, I'll move it to the edx-toggles pull request.

This PR is going to have less changes, as the Sphinx extension will be moved to edx-toggles. I am now considering the documentation of edx-platform settings. On this topic, I have two questions @robrap:

1. Should the documentation of edx-platform settings be part of the same documentation target? I think it should: currently, the docs are stored in edx-platform/docs/featuretoggles. I think it would make sense to rename this target to edx-platform/docs/reference (or edx-platform/docs/annotations).
2. If yes, should I append my changes to this PR or to a separate PR? There should not be too many changes, so I think we could bundle both toggles and settings docs in a single PR.

[robrap]

Thanks @regisb.

1. This PR still contains docs/featuretoggles/report.yml. It sounds like that can now be deleted?
2. Let's continue to have the settings conversation in Slack or Discourse, and not combine it. I'd like to land this and iterate. I understand that docs/featuretoggles may need to be renamed, but we can do that. I'm not sure yet what the right name is. docs/reference might be too general. docs/annotations is also too general, because I don't think we plan on reporting on .. pii annotations. Could possibly be docs/toggles-and-settings for now and renamed as necessary.

Also, do you imagine a Sphinx extension for .. featuretoggles:: and a separate one for settings, or a single extension like .. togglesandsettings::? The edx-toggles repo does not seem like the right place for the non-toggle settings annotation config yml. Where should this file live? Where should the Sphinx extension(s) live? If a single extension, could it work off of annotation config files pulled from multiple repos?

We may want to make a little headway on this discussion before landing the Sphinx extension PR, and before I work on getting readthedocs updated.

[regisb]

@robrap Currently, the pii annotation configuration file lives in edx-platform: https://github.com/edx/edx-platform/blob/master/.pii_annotations.yml
If we moved this file elsewhere, it would allow us to use it in other repositories.

To clarify: I propose that we rename edx-toggles to edx-POUAC where "POUAC" would be chosen wisely. The purpose of this repo would be to provide all Open edX-specific configuration files and tooling (including Sphinx extensions) around code annotations. Thus, we would move the .pii_annotations.yml annotations file there. The settings_annotations.yml file (which does not exist yet) would also be created there.

My vote for edx-POUAC goes to "edx-code-annotations". But if renaming the edx-toggles repo to edx-POUAC proved to be too much of a hassle, I would suggest we delay the decision and proceed to moving the configuration files anyway.

[robrap]

@regisb:

1. I think edx-code-annotations makes sense. Just want to float this by a couple more folks.
2. I get that POUAC was just a stand-in, but I’m curious what that means?
3. I think edx-toggles has its own purpose, including the future home of what is currently waffle_utils in edx-platform, so I think we want a new repo.

[regisb]

I get that POUAC was just a stand-in, but I’m curious what that means?

It's my own custom string for "foobar" :) (true story: I once pushed in production a change that appended "pouac" to all course names openfun/edx-theme@dafe88c Since then I have a git hook on my laptop to prevent me from accidentally commiting the "pouac" string)

I think edx-toggles has its own purpose, including the future home of what is currently waffle_utils in edx-platform, so I think we want a new repo.

I'm fine with that. In the meantime, I'll add the setting annotation configuration file and Sphinx extension in edx-platform.

[robrap]

@regisb: After discussing with my team, we are going to propose the following decision (which should be capture in an ADR):

• The repo edx/code-annotations will include both the tool, as well as the configs (yamls) and tools (sphinx extensions). Feanil proposed the term "profiles", be we need a folder structure that clearly separates this from the tool itself. We don't think it is necessary/worth-while to create a separate repo at this time, because we simply have too many repos to manage and that doesn't seem like an issue.

We need to discuss this with @bmedx and ensure that his team can get on board with this decision.

[bmedx]

It seems very weird to me to have edx-specific configuration in a general purpose tool's repo. Half of the use cases of the tool are similar to edx-lint (enforcing Django code annotations), the other half are similar to edx-developer-docs (generating documentation for code). I'd suggest that either of those might be a better place to store shared configuration.

[robrap]

Thanks for your thoughts @bmedx. I want to clarify that the proposal includes clearly delineating in the repository between the general purpose tool, and the Open edX specific usages and tools. The idea was also floated to have a separate repo like open-edx-code-annotations, but the general consensus was that:

• The code-annotations tool could still be used as a general tool, even if these usages were included in the same repo,
• In theory, some of the usages like PII and Toggles are equally reusable,
• The cost of maintaining additional repositories doesn't seem work it at this time.

You gave two other proposals that involve an existing repo:

• edx-lint: Doesn't feel right to me, especially given the doc tools.
• edx-developer-docs: This could potentially work, but I don't think @nasthagiri fully supports the existence of this repo.

I'm going to have @nasthagiri weigh-in, so hopefully we can quickly get to a decision and move on.

Thanks again.

[nasthagiri]

Thank you for this discussion. It reminds me of the need to prioritize ARCHBOM-1130: OEP on Github repo creation practices, which would include guidelines for the granularity and boundaries of our repositories.

Right now, there is a constant overhead cost for each repository that we need to maintain. We currently have multiple tiny repos that are not worthwhile to keep as standalone. So we need to have a strong case for the choices we make on repository boundaries.

I agree with the recommendation to keep edX.org specific data and assets separate from the Open edX platform repositories (and would be private repos). In this case, however, the "profiles" in question are not edX.org specific since they can be used by any Open edX instance.

My recommendation is to keep a single repository for annotation-related code, but do so with SOLID design patterns so there are clear boundaries within the repository. The requirements files can also be designed to support Interface-Segregation.

Regarding Documentation generation from code annotations, I do believe having Documentation reports is valuable and applicable to any code-annotation (including PII annotations). Given that, as @robrap points out, the code annotation profiles in consideration are potentially generally usable by any consumer (including non Open edX consumers).

[regisb]

I must say that I agree with @bmedx on this. The code-annotations repo is currently very generic, and it gives us the opportunity to use it in contexts that are related neither to edX.org or Open edX. After all, documenting large, language-agnostic code bases is a frequent use case. I would love to add more features to this tool and to advocate for its more widespread usage (on my own personal time of course), but it's going to be difficult to do so when it contains Open edX-specific configuration and documentation.

Of course, I understand the problems caused by having too many repositories to manage. Could we split the pear in half (as we say in French) by having a single code-annotations repository that would contain two python packages? The code-annotations package would remain as it is and we would add the edx-code-annotations package that would contain the Open edX-specific configuration files as well as the Sphinx extensions. This package would be pushed to pypi separately.

Would that be possible?

[robrap]

@regisb: Although it would be possible to split the packages, it was determined that the effort (including maintenance), wasn’t yet warranted.

Here is the ADR for using code-annotations for configs and tools: openedx/code-annotations#43

[regisb]

This should be ready to merge once we merge the corresponding PR in the code-annotations repo, where we moved the feature toggle annotation configuration file: openedx/code-annotations#44

[robrap]

Cool. I’ll want to test in readthedocs before merging either. I may not get to that until Monday because I’m mostly off until then.

[robrap]

@regisb: Please update the code-annotations reference and squash to your liking. Let me know when both are complete. Thanks.

[regisb]

@robrap I rebased my changes on top of master, removed the TODOs (including the reference to code-annotations from pypi), squashed the commits and updated this PR. As far as I am concerned, this is ready to merge.

[edx-status-bot]

Your PR has finished running tests. There were no failures.

[edx-pipeline-bot]

EdX Release Notice: This PR has been deployed to the staging environment in preparation for a release to production.

[edx-pipeline-bot]

EdX Release Notice: This PR has been deployed to the production environment.