RTD custom build#1096
Conversation
|
a5c63f4 to
bd62fd4
Compare
anton-seaice
left a comment
There was a problem hiding this comment.
Is this change only required so that there isn't one dead link when making a new page in documentation ?
Yes, any new pages. In general we would also have a proper |
anton-seaice
left a comment
There was a problem hiding this comment.
This is quite a complicated change for a very minor problem. This sort of customisation is difficult to understand and prone to breaking semi-regularly, so will add maintenance burden.
My first thought would be to not fix it. Is there consensus in @ACCESS-NRI/hivedocsteam to go ahead with this?
|
Thanks @atteggiani for proposing and working on this and @anton-seaice for your review. My understanding from the Hive Docs meeting yesterday was that fixing the pencil icon link is a nice improvement. But after looking at the details, it seems like the amount of custom code needed is pretty big, as already suggested by @anton-seaice, it increases ongoing maintenance load on us. In this case, I would agree with the review and suggest not to introduce a complicated solution for a minor feature. A few additional reasons that I could think of:
Tip Possible Solutions - We can still take a few lighter solutions:
Note And honestly, for someone like me who isn’t the code owner, it looks like it would take quite a while to understand how all of this works before even attempting to fix it 😂. |
KAUR1984
left a comment
There was a problem hiding this comment.
Added my review as a comment here - #1096 (comment)
I understand this adds some complication, but the problem is not minor, given that it would make the link-checker not usable . Why do you think the customisation is prone to breaking semi-regularly? To me it seems like a pretty solid implementation. We can certainly improve commenting and explanations and possibly take some more steps to make it more foolproof, but I don't see too many sources of errors in the current implementation. |
Link checking only fails when a new page is added right ?
It's reliant on many things from outside this repo and therefore not under out control. These are all prone to changing:
I'm not at all an expert on this stuff, maybe we need to seek some thoughts from other folk? |
Unfortunately the benefit is not small, because without this the link-checker would not be usable consistently. There will always be cases in which it will fail and we don't have ways to control that, meaning each maintainer of the documentation would need to know why it fails and "ignore" the error (but only if it only fails for the know error). This is a massive development burden for everyone developing a documentation website.
The only actual additions are ~40 lines of
I understand the pencil icon would not necessarily be needed for PRs, and in fact a solution would be removing that (it would still need a similar implementation but highly simplified). I think in general, despite the minor additions, this decreases the overall burden on development and makes it easier and quicker to setup another documentation website which uses the same infrastructure and "just works" as expected. |
|
I will abstain from this - I don't think this or the edit pencil button are important enough to be a priority. Navigating the git website is easy enough to figure out how to edit the docs. |
|
Thank you for your inputs @KAUR1984 and @anton-seaice. I agree the "pencil icon" is not a vital feature, but is one that allows easier development and with the Hive Docs team we discussed about adding it to the Hive Docs and other documentation websites. Also, this regards all documentation websites, for which in general the less manual changes required for specific websites the better. To fully take advantage of all these features, a custom RTD build is needed, to allow setting of needed env variables so the This custom RTD build is controlled by a single repo (this repo for the moment, but I'm thinking a separate dedicated repo might be created for that in the future) and this reduces a lot the development burden, because changes would only need to be applied one repo and would reflect in the build of all documentation websites. For the reasons above, please feel free to comment this PR with specific improvements for clarity and error-prone parts. I will also ask some more people to review, so we have a more comprehensive picture. |
5ebff2c to
0913f4c
Compare
CodeGat
left a comment
There was a problem hiding this comment.
Just some comments regarding the tooling that is being installed...
| gh_api_url="https://api.github.com/repos/$owner/$repo" | ||
| pr_info=$(curl -s "$gh_api_url/pulls/$READTHEDOCS_GIT_IDENTIFIER") | ||
| pr_head_owner=$("$JQ_EXE" -r '.head.user.login' <<< "$pr_info") | ||
| if [ "$owner" == "$pr_head_owner" ]; then # Not a fork | ||
| # Get the branch name | ||
| branch_name=$("$JQ_EXE" -r '.head.ref' <<< "$pr_info") | ||
| fi |
There was a problem hiding this comment.
Similarly, if this is running on GitHub Runners, gh is installed, and may be a bit easier to work with than calling the api via curl
There was a problem hiding this comment.
Same as above, unfortunately I don't think gh is available out of the box within AWS instances.
There was a problem hiding this comment.
Here the choice might be whether to download gh (and using jq within it), or download jq and use curl ... for API queries (as it is done currently).
Not sure if there's any preferred approach as we would always need to download something.
What do you think?
There was a problem hiding this comment.
Also, as a general approach, do you think it might make more sense to pin the versions of the external software we download instead of using latest?
There was a problem hiding this comment.
Hmm, I think if you can get away with using gh with it's inbuilt jq that would be cleaner. However note that you would need to find a way to pass a suitable GitHub PAT to gh to enable use of it.
And yes, pin versions of external software. It means one day when any of the tools has a breaking change, you won't need to spend time figuring out why the workflow broke!
There was a problem hiding this comment.
Yes, I remember now I chose to use curl over gh so I wouldn't need a PAT. The problem is that secrets cannot be set in RTD to be used by PRs. Secrets can only be used by "internal" builds, otherwise they will need to be set as plain env variables (i.e., they will be visible to anyone that has access to the RTD project settings/build, and that's definitely not what we want).
Therefore, I think I'll keep the use of jq and yq as it is, but update the script to pin specific versions.
Overview
Added custom build script to:
edit_uriat build timeNote
The current RTD build fails because the readthedocs config is trying to fetch for the
custom_build.shfile inmain(after 5ebff2c), but that file is added in this PR and therefore cannot be found inmain. This is the setup that will work in production.A successful test using the script fetched from this PR head branch can be found here.
More information about implementation
Currently this PR sets a custom build script (
custom_build.sh) to replace the default RTDbuildjob.This is necessary as env variables cannot be passed from a custom RTD job to another (separate RTD jobs are run with brand new environments and cannot inherit the enviroment from a previous custom job). This is the reason why the
EDIT_URIenv variable cannot be set in a custom job beforebuild(for examplepost-checkout) and passed to the default RTD build step.