-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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.
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
Config file v2 docs #4451
Config file v2 docs #4451
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is looking pretty good.
docs/config-file/index.rst
Outdated
|
||
.. note:: | ||
|
||
Using a configuration file is the recomended way of using Read the Docs, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
recomended -> recommended
docs/config-file/index.rst
Outdated
.. note:: | ||
|
||
Using a configuration file is the recomended way of using Read the Docs, | ||
the web interface would be deprecated, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we give a deprecation timeline? Should we urge that new projects should always use the config file?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure about the exact time. But Anthony mention that we will use a feature flag to block the ui, so new projects still can build with no configuration file, but if they need further configuration a config file is needed. I'm a little worried about projects moving to rtd (where older versions can't be updated)
@@ -42,7 +42,7 @@ Information about development is also available: | |||
features | |||
support | |||
faq | |||
yaml-config | |||
config-file/index |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is just a note that after merging this we should add a redirect for our docs (yaml-config.html
-> config-file/v1.html
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great addition! The only thing missing from the v2 docs is a warning "These features are still in development and are not yet supported" and maybe add a warning about v2 config is not enabled by default for projects yet. Alternatively, this PR just sits around until everything is ready for the v2 config and we've removed the feature flag requirement for use. I'd probably prefer to get this merged early and have warnings for users -- maybe this way we can get some users for testing.
docs/config-file/index.rst
Outdated
Configuration File | ||
================== | ||
|
||
Additionally to the admin panel of your project, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Additionally
-> In addition
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, probably just In addition to using the admin panel of your project to configure your project,
. Otherwise this sentence isn't clear.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't redundant to say of your project
twice?
docs/config-file/v2.rst
Outdated
|
||
Formats of the documentation to be built. | ||
|
||
* Type: ``list`` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, not sure if it makes sense to redo this doc or the v1 docs, but reST already has 2 list types that work better than DIY-ing a list type via a bullet list: definition lists[1] and field lists[2]. Instead of a bullet list, I think a field list works best here:
:Type: ``list``
:Options: ``htmlzip``, ``pdf``, ``epub``
:Default: ``[]``
The output is a bit more tuned than a bullet list, for example on our theme:
https://sphinx-rtd-theme.readthedocs.io/en/latest/demo/lists_tables.html#field-list
I know this pattern was copied from the original v1 docs, and they got this wrong as well, so not sure if we change either.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I love how field list is rendered
@agjohnson that was the original idea. I like the idea of having "test users" too. Let me know how you want to ship this :). Thanks for the review! |
docs/config-file/v2.rst
Outdated
@@ -1,8 +1,8 @@ | |||
Configuration File V2 | |||
===================== | |||
|
|||
Read the Docs support configuring builds with a YAML file. | |||
The file, ``.readthedocs.yml``, must be in the root directory of your project. | |||
Read the Docs support configuring your documentation builds with a YAML file. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missed this the first pass, support
-> supports
We talked more on this and i think settled on blocking this PR on merge until the v2 was publicly usable. We'll need to remember to come back to this PR for any doc updates for the config until then though. |
2b6841e
to
bb04aa6
Compare
689229b
to
7176e4b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks pretty solid! I like it!
Although, there are some changes that I'd like to see before merging this.
I found that sometimes we are using a global example and not a specific example for each option and sometimes we are using both. I'm not sure if this is on purpose.
What's best? Use both examples, use a mix (as it is right now) or use just the specific one?
docs/config-file/index.rst
Outdated
- The settings are per version rather than per project. | ||
- The settings live in your VCS. | ||
|
||
.. note:: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A .. tip::
makes more sense here to me.
docs/conda.rst
Outdated
|
||
conda: | ||
file: environment.yml | ||
Conda support is avalible using a :doc:`config-file/index`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we are removing the example completely, it would be good to link to the proper section of the config file that explains this. Otherwise, if we point the readers to the Index they will need to start from the beginning.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is something I was thinking a lot, now that we have two versions, is kind of tricky to point to a specific one :/. Maybe always linking to the latest version?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. I think we should point the user to the one we want them to use. In this case v2
.
docs/config-file/v1.rst
Outdated
|
||
.. warning:: This feature is in a beta state. | ||
Please file an `issue`_ if you find anything wrong. | ||
.. note:: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
.. warning::
is better here.
Also, this block should mention that v1
shouldn't be used.
docs/config-file/v1.rst
Outdated
|
||
The version 2 of the configuration file is now avaliable. | ||
See the :doc:`new features and how to migrate from v1 <v2>`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Link to the proper section for migration here instead of the root of v2
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe two links are needed in this sentence:
- new features
- how to migrate from v1
docs/config-file/v2.rst
Outdated
.. note:: | ||
|
||
Not all settings in the web interface are per version, but are per project. | ||
Those settings aren't supported via the configuration file (like ``Default branch``). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suppose we should have a specific list with all the settings that can't be managed via the YAML file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need to check the admin panel p:
docs/doc_extensions.py
Outdated
@@ -6,11 +6,19 @@ | |||
djangosetting | |||
Output an inline literal of the corresponding setting value. Useful for | |||
keeping documentation up to date without editing on settings changes. | |||
|
|||
supportedversions |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd like to have a more descriptive name for this, like dockerpyversions
or buildpyversions
or dockerimagepyversions
or similar.
supportedversions
is too general.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
buildpyversions
I like this one p:
@@ -10,7 +10,8 @@ there are a couple fixes that you might try. | |||
Reduce formats you're building | |||
------------------------------ | |||
|
|||
You can change the formats of docs that you're building with our YAML file's :ref:`yaml-config:Formats` option. | |||
You can change the formats of docs that you're building with our :doc:`/config-file/index` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Link to the specific format option.
requirements_file: requirements.txt | ||
|
||
See :doc:`yaml-config` for setting up the .yml file | ||
The recommended approach for specifying a pip requirements file is to use a :doc:`/config-file/index` file. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Link to the specific section.
b2592d8
to
16e5a1e
Compare
bccd18a
to
240e513
Compare
This shouldn't be blocked anymore, but I think we should merge it after doing a deploy on .org |
:Type: ``number`` | ||
:Default: ``3`` | ||
|
||
python.install |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not really sure if this whole section is clear. Now that we accept a list with different keys.
This is ignored if there aren't submodules to clone. | ||
|
||
Schema | ||
------ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if this section is relevant
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's OK!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I noted some things, request some changes and made some suggestions that we should discuss before merging this.
Some of them are related with the implementation itself also and affects default values (#5155)
|
||
Install packages from a requirements file. | ||
|
||
The path to the requirements file, relative to the root of the project. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The path to the requirements file must be relative to the root of the project.
(added "must be" and removed the comma)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could be a .. note::
after showing the example. I think it will be clearer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a description of the key below, it also works as a separator for the list of keys, I wasn't sure how to list them
|
||
:Key: ``requirements`` | ||
:Type: ``path`` | ||
:Required: ``true`` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This key python.install.requirements
it's not required as far as I know.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What it means here that can't be empty. Again, not sure how to list the values for the install
key
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't compile the docs, but reading it from the diff I found they are good.
I wouldn't use :Required:
on these cases, though. I think it's confusing like "I need to have a requirements.txt to build my docs"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, I just put a default value to the whole python.install
, hope that make it more clear.
Migrating from the web interface | ||
-------------------------------- | ||
|
||
This should be pretty straightforward, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reading section is not too clear what I have to do as a user.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Find the settings that match from the web interface
|
||
See :doc:`/yaml-config` for setting up the .yml file | ||
The recommended approach for specifying a pip requirements file is to use a :doc:`/config-file/index` file, | ||
see :ref:`config-file/v2:Requirements file`. | ||
|
||
Using the project admin dashboard | ||
--------------------------------- |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We may suggest the user that it's preferable to use the config file here, I suppose.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We just mention that above
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I took a peek, but will wait until @humitos's feedback is addressed to give it a full review, so we don't double up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should ship this, and we can deal with the smaller changes as we get feedback from users. 👍
|
||
This should be pretty straightforward, | ||
just go to the :guilabel:`Admin` > :guilabel:`Advanced settings`, | ||
and find their respective setting in :ref:`here <config-file/v2:Supported settings>`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we generate a config automatically from their existing DB entries? It would be neat to go to /dashboard/project/yaml/
and get a generated YAML file with their existing build config.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This shouldn't block this PR, but would be a 💯 migration step.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, we actually need to do it from version. We have this issue opened #4746
We need to add a redirect after this is merged: |
You can add it now, and it will be ignored til it 404's. |
I don't have permissions there p: |
You do now! 🎉 |
@stsewd we need to remove the feature flag now, I think? |
Closes #4279
This should be merged as the last step of the configuration file project. We need to add a redirect after this is merged (
yaml-config.html
->config-file/v1.html
).I added a changelog to facilitate the migration from v1 and also a link from v1 to v2