openedx / edx-cookiecutters

Open edx public templates for apps, libraries and services.
Apache License 2.0
26 stars 22 forks source link

Readthedocs config template is broken #416

Closed robrap closed 9 months ago

robrap commented 10 months ago

The file https://github.com/openedx/edx-cookiecutters/blob/master/python-template/%7B%7Bcookiecutter.placeholder_repo_name%7D%7D/.readthedocs.yml gets complaints from readthedocs.

I fixed using more of the template from edx-platform, but I'm not sure of all the parts we want: https://github.com/openedx/edx-platform/blob/master/.readthedocs.yaml. Note that the requirements path is different, so don't blindly copy-paste.

This is from the readthedocs documentation: https://docs.readthedocs.io/en/stable/config-file/v2.html

Lastly, the cookiecutter itself is missing this top-level file, which means its build would probably be broken. Whether as a separate ticket or this ticket, we should probably add that file and publish the cookiecutter docs.

robrap commented 10 months ago

Readthedocs template:

# Optional but recommended, declare the Python requirements required
# to build your documentation
# See https://docs.readthedocs.io/en/stable/guides/reproducible-builds.html
# python:
#   install:
#     - requirements: docs/requirements.txt

edx-platform version of this:

python:
  install:
  - requirements: "requirements/doc.txt"
  - method: pip
    path: .

Do we need the extra bits from the edx-platform template, or is that an edx-platform specific need?

robrap commented 10 months ago

I see that the extra bits for edx-platform were added in https://github.com/openedx/edx-platform/pull/32752/files. Maybe we can just see if edx-drf-extensions (recently updated here) would still pass without these final two lines?

dianakhuang commented 10 months ago

I decided to add the extra lines to our cookiecutter config just to be on the safer side.

robrap commented 10 months ago

I'm going to reopen for now. See https://github.com/openedx/edx-cookiecutters/pull/419#issuecomment-1843715448. We may need to add additional python install details.

robrap commented 10 months ago

@feanil: The lighter version that you used in this PR worked fine in https://github.com/openedx/event-routing-backends/pull/374. I didn't look into this further, but it would be nice to know what version is needed for what, and to have it documented easily (commented out lines?) for those that will need the extra lines.

UPDATE: @feanil gives some explanation here: https://github.com/openedx/edx-cookiecutters/pull/418#discussion_r1417968346

feanil commented 10 months ago

Yea, thinking more through it, I think it does in fact make sense to put the pip install . into the .readthedocs.yaml file in the cookiecutter since the cookiecutter is for python projects and will have a setup.py by default. I created https://github.com/openedx/edx-cookiecutters/pull/420 to add it in along with some comments.

feanil commented 9 months ago

Given the above mentioned PRs to correct the cookiecutter, I think it's okay to close this again.