kiwix / kiwix-tools

Command line Kiwix tools: kiwix-serve, kiwix-manage, ...
https://download.kiwix.org/release/kiwix-tools/
GNU General Public License v3.0
408 stars 79 forks source link

User documentation of kiwix-serve #586

Closed veloman-yunkan closed 1 year ago

veloman-yunkan commented 1 year ago

Fixes #498.

kelson42 commented 1 year ago

@veloman-yunkan I wonder if the doc can be put on Readthedocs.io? Or how it should be made readable for people?

veloman-yunkan commented 1 year ago

This is the first cut.

I didn't document the KIWIX_SERVE_CUSTOMIZED_RESOURCES environment variable since it was introduced in order to simplify the development process and despite some potential for UI customization shouldn't be advertised before we all agree that it is a good idea.

Also, I didn't include any information from the last column of @rgaudin's table from https://github.com/kiwix/kiwix-tools/issues/498#issuecomment-1337309380 . Does it have to be reflected in the documentation?

kiwix-serve man page is not (yet) updated by this PR. I think it must be done in the end or as a separate PR.

veloman-yunkan commented 1 year ago

@veloman-yunkan I wonder if the doc can be put on Readthedocs.io?

@kelson42 Yes, similarly to how it was done for libzim. Do we have a kiwix account with readthedocs? Or @mgautierfr used his own?

kelson42 commented 1 year ago

@mgautierfr You are best to answer IMO, I'm not informed about an account for kiwix.

veloman-yunkan commented 1 year ago

I am concerned by the fact your are using an outdated Sphinx version (3.5.4, approaching 2 years old).

@rgaudin That is not a deliberate choice. Without having any prior experience with sphinx I just copied the setup from libzim.

rgaudin commented 1 year ago

I am concerned by the fact your are using an outdated Sphinx version (3.5.4, approaching 2 years old).

@rgaudin That is not a deliberate choice. Without having any prior experience with sphinx I just copied the setup from libzim.

Ah Ok. Well I've tested with latest stable and it works as well so you can use:

Sphinx==5.3.0
sphinx-rtd-theme==1.1.1

Actually, if you remove the reference to the theme in conf.py, you can limit it to Sphinx==5.3.0. It would use the new theme which is certainly different but is more readable I believe.

mgautierfr commented 1 year ago

@mgautierfr You are best to answer IMO, I'm not informed about an account for kiwix.

There is no kiwix account. Administration of projects on readthedocs are made through your github account (you can log with them on readthedocs)

Another comment is that the doc option doesn't seem to be set on CI builds right? I think we should build it to ensure it continues to pass. But maybe you'll build and upload to readthedocs on a dedicated workflow?

readthedocs pull and compile the documentation so we don't have any CI to do on our side.

I've just activated the project and create a temporary "hidden" documentation for this branch : https://kiwix-tools.readthedocs.io/en/documentation/

As this doesn't compile for the reason exposed by @rgaudin, this is now a 404 but it will change soon.

About the dependencies versions:

The pinning of sphinx<4 is pretty old. It seems to works on libzim has readthedocs has its own pinning of jinja2<3.1.0. However, it is not the case for libkiwix and kiwix-tools. This is probably the "hot fix" in https://github.com/readthedocs/readthedocs.org/issues/9038#issuecomment-1080481986

On libkiwix I have fixed this by not pinning sphinx<4 (https://github.com/kiwix/libkiwix/pull/736) I suggest to remove the sphinx pinning here also for consistency with libkiwix. (But I'm still open for counter arguments to pin our versions)

rgaudin commented 1 year ago

I wonder if we should document all endpoints publicly. Having a documentation for a endpoint de facto create a contract between the developers and the user that they can use the api.

I'm in favor of documenting them (kind of the point of the initial ticket) but clearly mark them private or whichever wording implying it being an exposed endpoint but not a supported API.

mgautierfr commented 1 year ago

.... whichever wording implying it being an exposed endpoint but not a supported API.

Yes. I think you point the right thing here : an endpoint may not be part of a API. It is open question if we should document them. We don't document private method when documenting cpp/python code[*], we may decide to not document private endpoint.

Maybe we can move the private endpoint in a specific page "private endpoints" and move the pulbic api into a dedicated "http public api" page ?


[*] We may put comment in the code for us but they are not listed in the generated (html) documentation.

veloman-yunkan commented 1 year ago

Well I've tested with latest stable and it works as well so you can use:

Sphinx==5.3.0
sphinx-rtd-theme==1.1.1

Actually, if you remove the reference to the theme in conf.py, you can limit it to Sphinx==5.3.0. It would use the new theme which is certainly different but is more readable I believe.

@rgaudin Thanks! I preferred keeping the RTD theme, because of its multilevel table-of-contents in the sidebar.

I've just activated the project and create a temporary "hidden" documentation for this branch : https://kiwix-tools.readthedocs.io/en/documentation/

As this doesn't compile for the reason exposed by @rgaudin, this is now a 404 but it will change soon.

@mgautierfr Thank you too! This PR is now live at https://kiwix-tools.readthedocs.io/en/documentation/

veloman-yunkan commented 1 year ago

The concerns related to the public/private endpoint separation have not been addressed. Let's decide which approach we are going to take.

holta commented 1 year ago

Great, Thanks All !

kelson42 commented 1 year ago

LGTM ; Do we want to settle the public/private in this PR or in a following one?

Here my feedback on this subject. I´m not really confortable with the idea of having two levels (unofficial/official, private/public, ...) of APIs. I'm not principly opposite to it, but it sounds really complicated for what we need and I see potential problems appearing because of this.

I would suggest that we open a dedicated ticket (before merging that one) for the end-points we are not sure to put in public documented API. We could then discuss if:

These 3 options seems to me good enough (but maybe the future discussion will show that I'm wrong).

rgaudin commented 1 year ago

I don't have a strong opinion on whether we should have unsupported endpoints or not. I can see how it gives us flexibility but I can also see how we could allow ourselves to break API at any version, given we document changes and respect semantic versioning.

I do have one on documentation though ; as not documenting all the endpoints (ie. hiding somes) would not solve my use cases. I would definitely prefer documented-yet-marked-unsupported ones.

Documenting means being transparent and transparency helps integration and maintenance. Kiwix-serve is critical to a number of our products and other organization ones. By documenting an endpoint (even as unsupported), we acknowledge that it exists and can mention its change or removal in Changelog and that will be beneficial to all.

Hiding it won't prevent users from using it because they might find it anyway because they need something that's not in the public API… but it will break without notice and generate frustration and will discourage them from giving feedback.

rgaudin commented 1 year ago

About ZIMID : There is a lot of ZIMID referencing the term ZIM name. I think it would be better to rename them to ZIMNAME.

Some endpoint accepts the ZIM UUID while others accepts the Name (not always the metadata Name as libkiwix generates it in some cases) and even one accepts the Name without the period suffix.

The documentation should be clear about which one(s) is accepted. This is currently not the case.

For instance, it states

curl https://library.kiwix.org/raw/ubongo_sw_playlist-mziki-wa-ubongo-kids_2021-06/meta/Title
curl https://library.kiwix.org/catalog/v2/entry/00d73467-0b73-08d8-7577-48e1206b7d18
mgautierfr commented 1 year ago

I would suggest that we open a dedicated ticket (before merging that one) for the end-points we are not sure to put in public documented API.

Done in #593

veloman-yunkan commented 1 year ago

@rgaudin

About ZIMID : There is a lot of ZIMID referencing the term ZIM name. I think it would be better to rename them to ZIMNAME.

Some endpoint accepts the ZIM UUID while others accepts the Name (not always the metadata Name as libkiwix generates it in some cases) and even one accepts the Name without the period suffix.

The documentation should be clear about which one(s) is accepted. This is currently not the case.

I have specifically linked all references to ZIM name and UUID to their descriptions in the glossary.

For instance, it states

  • /raw/ZIMNAME/meta/METADATAID (actually accepts Name)

To the best of my understanding, usage of ZIMNAME here is correct and no different from its usage elsewhere.

  • /catalog/v2/entry/ZIMNAME (actually accepts UUID)

Fixed this one.

rgaudin commented 1 year ago

OK I suppose it was that one mistake that got me thinking it wasn't specified. I was skimming through the PR UI and did not rebuilt the doc. Thanks

veloman-yunkan commented 1 year ago

Fixed the issue noticed by @rgaudin (https://github.com/kiwix/kiwix-tools/pull/586#discussion_r1060052999) and also documented usage of cacheid with /skin.

veloman-yunkan commented 1 year ago

A badge linking to the doc should be added in the README.md, like on libzim.

Oops, missed this comment. Done now.

kelson42 commented 1 year ago

@mgautierfr @veloman-yunkan After a rebase (and fixing the conflicts) are we good to merge?

mgautierfr commented 1 year ago

After a rebase (and fixing the conflicts) are we good to merge?

There is still the question of how we document the private endpoints. The question may be delayed for later (and so we merge the PR) but we MUST answer the question (and update documentation) before the next release.

kelson42 commented 1 year ago

After a rebase (and fixing the conflicts) are we good to merge?

There is still the question of how we document the private endpoints. The question may be delayed for later (and so we merge the PR) but we MUST answer the question (and update documentation) before the next release.

593 is indeed plan in next release 3.5.0. It should not be considered a blocker for this PR.

But rebasing + CI working is mandatory.

In addition https://kiwix-tools.readthedocs.io/en/latest/?badge=latest is empty, not sure this is normal.

mgautierfr commented 1 year ago

In addition kiwix-tools.readthedocs.io/en/latest/?badge=latest is empty, not sure this is normal.

Yes. PR are not included in "latest". Once merged, it should be ok.

veloman-yunkan commented 1 year ago

But rebasing + CI working is mandatory.

Rebasing (combined with some history clean-up) done. CI is "clean" (Packages workflows won't pass until the libkiwix-dev and libzim-dev packages in http://ppa.launchpad.net/kiwixteam/dev/ubuntu are upgraded to newer releases).

kelson42 commented 1 year ago

@mgautierfr Good to merge imo