canonical / discourse-gatekeeper

Experimental GitHub Action to upload charm documentation to charmhub
Apache License 2.0
7 stars 7 forks source link

Support external links #212

Closed jdkandersson closed 10 months ago

jdkandersson commented 10 months ago

Applicable spec: https://discourse.charmhub.io/t/specification-isd003-upload-charm-docs-contents-index-and-navigation-table/7986

Overview

Adds support for external links on the contents index which are then written to the navigation table.

Rationale

Including external links enables them to be added to the navigation table which is a requirement for documentation.

Module Changes

Checklist

Version should be incremented on release

jdkandersson commented 10 months ago

I started to go through the PR, but then I started to have some thoughts about what we mean by external/internal link.

I believe Discourse and frontend platforms (e.g. charmhub) allow for different behaviour when one provides

[Link](https://discourse.com/t/my-topic)

and

[Link](/t/my-topic)

The first does represent an actual re-direct to Discourse, the second is a reference which is handled by the frontends (e.g. charmhub) by rendering the other content, still within the platform.

So my point here is that https://discourse.com/url may possibly still be treated as an external link, regardless of whether it starts with the server name or not. This may also allow in the README ( (consistently with the different behaviour in charmhub) to refer to discourse documentation outside of the scope of the product, without copying/rendering the content in Github, e.g. :

[Content In Docs]("/tutorial/doc.md") # Link rendered in Github
[Content In Other Docs]("https://dicourse.com/tutorial/doc.md") # Link redirected to Discourse

@deusebio An external link is intended to be something that isn't a link to a topic that should be rendered as documentation. You are right that this could be a discourse topic which would currently get treated as a documentation topic. The current approach to handling URLs is that, during execution, the full URL to the topic is used for discourse interactions. So in many cases the distinction between a discourse topic intended as documentation and an external link to a discourse topic would be lost. This indicates that there is a refactor needed of the URL handling which I would propose for a follow up PR along with cleaning up the URL generation in tests. For now, this adds support for linking, e.g., to the configuration or actions from the navigation table which is commonly done and recommended so it represents progress wghich is why I suggest we proceed with it.

github-actions[bot] commented 10 months ago

Test coverage for 4d1d52ba83db9239c24010f5f860699223087f88

Name                      Stmts   Miss Branch BrPart  Cover   Missing
---------------------------------------------------------------------
src/__init__.py              83      0     34      0   100%
src/action.py               155      0     46      0   100%
src/check.py                 53      0     21      0   100%
src/clients.py               12      0      0      0   100%
src/commit.py                42      0     12      0   100%
src/constants.py             10      0      0      0   100%
src/content.py               50      0     10      0   100%
src/discourse.py            159      0     34      0   100%
src/docs_directory.py        33      0      8      0   100%
src/download.py              23      0      2      0   100%
src/exceptions.py            15      0      0      0   100%
src/index.py                142      0     56      0   100%
src/metadata.py              28      0     12      0   100%
src/migration.py             87      0     27      0   100%
src/navigation_table.py      65      0     20      0   100%
src/reconcile.py            123      0     60      0   100%
src/repository.py           284      0     88      0   100%
src/sort.py                  43      0     26      0   100%
src/types_.py               196      0     54      0   100%
---------------------------------------------------------------------
TOTAL                      1603      0    510      0   100%

Static code analysis report

Working... ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ 100% 0:00:02
Run started:2023-09-25 08:40:35.599643

Test results:
    No issues identified.

Code scanned:
    Total lines of code: 17032
    Total lines skipped (#nosec): 3
    Total potential issues skipped due to specifically being disabled (e.g., #nosec BXXX): 0

Run metrics:
    Total issues (by severity):
        Undefined: 0
        Low: 0
        Medium: 0
        High: 0
    Total issues (by confidence):
        Undefined: 0
        Low: 0
        Medium: 0
        High: 0
Files skipped (0):
deusebio commented 10 months ago

@jdkandersson Yes, that's fine. I thought that changing the logic would have been only on the is_external function and fairly self-contained and straightforward, but - yet - I believe it is important to define what external means. It is fine to approach this in a subsequent PR if this means a substantial rework in this PR, which is more around adding support for external link, which I agree and I believe it is sensible.