-
Notifications
You must be signed in to change notification settings - Fork 14.4k
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
Fix links to sources for examples #24386
Conversation
CC: @josh-fell - another fix to links for AIP-47 vs. I think fixed past broken links will be difficult - because we would have to regenerate a lot of the class "apis" which were excluded (mistakenly) but I have an idea how we can instead utilise links to github and post-process links in the generated documenation (very similar to the fix merged yesterday) |
22d66f6
to
ed85209
Compare
cc: @rbiegacz |
ed85209
to
414825d
Compare
@mik-laj - finally got some time to take a look and It looks I managed to fix it :) |
@mik-laj :) ? |
414825d
to
278cc7f
Compare
278cc7f
to
060f78d
Compare
I also excluded docs from the "no relative imports rule" and converted the imports there to be relative in order to make "docs building utils" work fine with scripts and flake/isort without having to do some "noqa: excludes" @mik-laj. |
060f78d
to
2e76e27
Compare
@@ -33,6 +33,7 @@ Content | |||
:caption: References | |||
|
|||
Python API <_api/airflow/providers/apache/cassandra/index> | |||
System Tests <_api/tests/system/providers/apache/cassandra/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.
Why are we linking to this from user facing docs? That seems to me like a very odd thing to do.
In other words: I can't think of any reason why a user would ever visit and git anything useful from this page.
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 also thought we might not need that. It's Mainly there add it to any toc tree- if we don't do it, sphinx complains that the index is missing from the toc tree. Do you know how to exclude those links in general? I am happy to remove them, but I simply have no idea how. Likely some exclude somewhere in sphinx config.
Previously example dags were excluded by */example_dags/*
and that's why it was not needed but with the latest sphinx update it caused hte problem of missing examples. We cannot exclude them this way because then they will be removed from the generated html (that was precisely the root cause of missing links previously - the "missing index in toc tree" was not generated but also link to example dags led to 404 because the generated example_dags pages were missing.
So if we know a better way to keep the files but not to have them included in any toc tree - that would be awesome. But I do no know how to do it.
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.
Ok I think I found a way.
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.
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 would be too easy.
But I am close to get it (and I also found some index inconsistencies that I am also fixing).
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.
FYI. @ashb The main problem was that :hidden: in toc tree did not remove the sidebar link. And I found out in the theme we can disable sidebar links for hidden TOC, but we were already using this in "apache-airfow" and "chart" (but luckily only there) to show sidebar content while not showing TOC in the page.
Now we have hidden links shown in the sidebar in Airflow:
And no "system tests" links in provider's - neither sidebar, nor main page:
But the sources are there.
Both for legacy exmple dags:
And for new system tests:
Also we have consistent "example dags" links
Wasn't THAT straightforward :). But finally seems to be there.
I also found a few inconsistencies in provider indexes that I fixed. The system tests for google providers were moved without sub-folders (which was wrong).
The __init__.py
works nicely for docs scripts too. While enabling relative imports in "docs" I managed to make it in the way that:
- no need to insert syspath before imports
- build_docs.py ./publish_docs.py still works with shebang as executables and you can run it either with local airflow venv or in breeze for repeatability
- isort behaviour is consistent no matter if you only modify docs/* python files or run pre-commit for --all-files
- we have no implicit package in docs.
I also double-checked that adding init.py in docs did not mess with the airflow package (which I was afraid that could be side effect:
⌂85% θ92° [jarek:~/code/airflow] [airflow-3.7] fix-source-links-in-examples(+15/-1)+ 41s ± unzip -t dist/apache_airflow-2.4.0.dev0-py3-none-any.whl| grep doc
testing: airflow/contrib/operators/docker_swarm_operator.py OK
testing: airflow/hooks/docker_hook.py OK
testing: airflow/operators/docker_operator.py OK
testing: airflow/providers/docker/__init__.py OK
testing: airflow/providers/docker/decorators/__init__.py OK
testing: airflow/providers/docker/decorators/docker.py OK
testing: airflow/providers/docker/hooks/__init__.py OK
testing: airflow/providers/docker/hooks/docker.py OK
testing: airflow/providers/docker/operators/__init__.py OK
testing: airflow/providers/docker/operators/docker.py OK
testing: airflow/providers/docker/operators/docker_swarm.py OK
testing: airflow/utils/docs.py OK
testing: airflow/www/static/dist/redoc.standalone.js OK
testing: airflow/www/static/dist/redoc.standalone.js.LICENSE.txt OK
testing: airflow/www/static/dist/redoc.standalone.js.map OK
testing: airflow/www/templates/airflow/redoc.html OK
testing: airflow/www/templates/appbuilder/dag_docs.html 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.
eeechhh. Bash operator is still not there :(
928c91e
to
742cf81
Compare
'**/example_sla_dag.py', | ||
'**/example_taskflow_api_etl_docker_virtualenv.py', | ||
'**/example_dag_decorator.py', |
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.
These files had problems/not used, right?
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 can try to remove those and see. :)
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.
Pretty cryptic:
============================== apache-airflow ==============================
------------------------------ Error 1 --------------------
WARNING: Unknown type: placeholder
------------------------------ Error 2 --------------------
WARNING: Unknown type: placeholder
When I remove them. I will leave to the next poor soul that will start thinking that fixing docs building MUST be easy.
The PR most likely needs to run full matrix of tests because it modifies parts of the core of Airflow. However, committers might decide to merge it quickly and take the risk. If they don't merge it quickly - please rebase it to the latest main at your convenience, or amend the last commit of the PR, and push it with --force-with-lease. |
The links to example sources in exampleinclude have been broken in a number of providers and they were additionally broken by AIP-47. This PR fixes it. Fixes: apache#23632 Fixes: apache/airflow-site#536
742cf81
to
e3745cc
Compare
Merging quickly to fix docs-buiild error introduced in #24413 - it built except that doc error |
The links to example sources in exampleinclude have been broken in a number of providers and they were additionally broken by AIP-47. This PR fixes it. Fixes: apache#23632 Fixes: apache/airflow-site#536 (cherry picked from commit 08b675c)
The problem (fixed in apache#24386) with broken links was that the autoapi-generated source html were either exluded (via example_dags/** exclusion) or not included (system_tests) when the documentation was generated. We cannot easily re-add and regenerated all those autoapi sources so instead the fix is to replace all the links to autoapi sources with corresponding links to raw github sources. We can easily do that using tags of providers and link to specific tagged versions of those example dags and this is the best we can do now. The apache#24386 brings back linking to autoapi-generated html pages.
The links to example sources in exampleinclude have been broken in a
number of providers and they were additionally broken by AIP-47.
This PR fixes it.
Fixes: #23632
Fixes: apache/airflow-site#536
^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code change, Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in a newsfragement file, named
{pr_number}.significant.rst
, in newsfragments.