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.

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

docs: forward-ports from 3.x #256

Merged
merged 20 commits into from
Jul 24, 2024

Conversation

tchaikov
Copy link

@tchaikov tchaikov commented Nov 9, 2023

the document on 4.x branch does not build without this changeset. in order to build docs from 4.x branch, we have to have cdaf75a. but since we are here, let's forward-port all 3.x doc change to 4.x.

tested using

$ make setup
$ make preview

@tchaikov
Copy link
Author

tchaikov commented Nov 9, 2023

/cc @dgarcia360 @annastuchlik

@roydahan
Copy link
Collaborator

roydahan commented Jan 4, 2024

Need to rebase PR in order to re-run the CI.

@tchaikov
Copy link
Author

tchaikov commented Jan 5, 2024

rebased and repushed.

@avelanarius
Copy link

@dgarcia360 @annastuchlik Please review this PR.

@tchaikov I'm not really sure I understand the reasoning for this PR. The cover letter doesn't really explain the intention.

@tchaikov
Copy link
Author

@dgarcia360 @annastuchlik Please review this PR.

@tchaikov I'm not really sure I understand the reasoning for this PR. The cover letter doesn't really explain the intention.

@avelanarius hi Piotr, the document on 4.x branch does not build without this changeset. that's why i ported the change to address the FTBFS.

@dgarcia360
Copy link

@annastuchlik @tchaikov @avelanarius It looks good to me. The issue is that, without these changes, the 4.x will not build locally.

@roydahan roydahan requested a review from Bouncheck April 25, 2024 11:31
@roydahan roydahan assigned Bouncheck and unassigned avelanarius Apr 25, 2024
@roydahan
Copy link
Collaborator

Ping reviewers

Copy link
Collaborator

@Bouncheck Bouncheck left a comment

Choose a reason for hiding this comment

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

I successfully ran make preview after checking out this branch, however I've seen that script had to downgrade a lot of dependencies. Maybe a rebase is needed.
Github workflows seem incorrect or unnecessary: do we do docs deploy from 4.x branch? Deploy.sh seems not to be there anyway.
Is this purely a port? CustomCommonMarkParser seems to be new, do we not need it in 3.x?
Branchest/Tags need an update: latest tag is 4.18.0.1

on:
push:
branches:
- scylla-3.x
Copy link
Collaborator

Choose a reason for hiding this comment

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

This feels wrong. Wouldn't that never fire?

on:
pull_request:
branches:
- scylla-3.x
Copy link
Collaborator

Choose a reason for hiding this comment

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

Similar as above. We should not have a workflow for PRs to 3.x on 4.x branch.

@Bouncheck
Copy link
Collaborator

How do we maintain those two docs branches going forward? Is it possible to make it so we keep up to date setup on scylla-3.x and from time to time copy whole docs subdirectory to scylla-4.x?

avelanarius and others added 13 commits July 11, 2024 10:43
Before this change, the docs failed to build with the following
error:

ImportError: cannot import name 'environmentfilter' from 'jinja2'

A fix to this problem is to explicitly pin Jinja2 version.

(cherry picked from commit cdaf75a)
docs: update dynamic slug

Add back index.md

Move custom slug to a separate PR

Fix typo

(cherry picked from commit 8feeb20)
Add Java Driver 4.x branches to the list of generated documentation
and bump the LATEST_VERSION variable.

(cherry picked from commit 76f7715)
(cherry picked from commit d2e261b)
Fix javadoc redirect

Fix

(cherry picked from commit a9fcffb)
(cherry picked from commit 7399a0a)
The links in our documentation have a hardcoded version number in them:

https://java-driver.docs.scylladb.com/scylla-3.11.2.x/api/
https://docs.datastax.com/en/drivers/java/3.11/com/datastax/driver/core/Cluster.html

Therefore, a replacement rule is needed to replace the version to a
correct version for a specific driver version we render the docs for.

Before this change, only datastax links were replaced. After this
change, java-driver.docs.scylladb.com links are also replaced.

(cherry picked from commit 8cd4d8e)
Add 4.14.1.x and 4.15.0.x releases to the list of versions to generate
the docs for.

(cherry picked from commit d296895)
docs: update conf.py

docs: update conf.py

docs: Update conf.py

docs: update conf.py

docs: Update conf.py

docs: update conf.py
(cherry picked from commit 0d1f1c4)
docs: test

docs: add skip warnings option
(cherry picked from commit 2601b78)
(cherry picked from commit 51ff0d5)
(cherry picked from commit 7ab2a8f)
@tchaikov
Copy link
Author

tchaikov commented Jul 11, 2024

I successfully ran make preview after checking out this branch, however I've seen that script had to downgrade a lot of dependencies. Maybe a rebase is needed.

sure. rebased.

Github workflows seem incorrect or unnecessary: do we do docs deploy from 4.x branch?

i didn't manually edit the backport commits unless i had to to resolve the conflicts or to fix the document build. the purpose of this changeset is to enable us to build document on 3.x branch. i am not sure if we deploy docs from 4.x branch. guess this is a question to the maintainer of this project.

considering this PR has been around for half a year, can we get this changeset reviewed and then work on the CI of document deployment if this is desirable?

Deploy.sh seems not to be there anyway. Is this purely a port?

yes, it is a backport. but again, this PR has been around for ages. and i haven't cherry-picked new changes from 4.x since i sent it for review. if something is odd. the reason is very likely that it's caused by merge conflict or missing commits from 4.x.

CustomCommonMarkParser seems to be new, do we not need it in 3.x?

on the contrary, it is old. the change removing CustomCommonMarkParser landed in 9eedcac

Branchest/Tags need an update: latest tag is 4.18.0.1

could you be more specific? which files shall i update?

@tchaikov
Copy link
Author

tchaikov commented Jul 11, 2024

v2:

@tchaikov
Copy link
Author

How do we maintain those two docs branches going forward?

again, i think it's up to the maintainer of this project.

Is it possible to make it so we keep up to date setup on scylla-3.x and from time to time copy whole docs subdirectory to scylla-4.x?

i don't think to "copy" the whole docs subdirectory to scylla-4.x branch is the right approach. my 2 cents would be

  1. get the 4.x doc build in shape, so that it can at least build
  2. decide if / how to deploy it
  3. setup the CI to backport and deploy it
  4. add the proper label when sending doc patches to 3.x, and let CI to backport the change.

tchaikov and others added 5 commits July 12, 2024 08:34
Signed-off-by: Kefu Chai <[email protected]>
(cherry picked from commit 6a5d3a7)
(cherry picked from commit 8dea9e2)
(cherry picked from commit 9eedcac)
this change is not cherry-picked from 4.x, as it is specific to
scylla-3.x branch.

Signed-off-by: Kefu Chai <[email protected]>
(cherry picked from commit efe5101)
@tchaikov tchaikov force-pushed the 4.x-docs-backports branch 2 times, most recently from de2c484 to 8d881b2 Compare July 12, 2024 01:45
@tchaikov
Copy link
Author

@Bouncheck hi Wojciech, could you please take another look?

@Bouncheck
Copy link
Collaborator

Bouncheck commented Jul 12, 2024

make clean preview looks better. Now manual pages can be browsed normally (before they did not work). API Documentation does not link to javadoc.

make clean multiversionpreview does not build correctly for me. For testing I've removed all but 2 versions (4.14.1.x and 4.15.0.x) from BRANCHES in source/conf.py to speed up the build. I'm getting a 404 error page instead.
Throughout the build I'm getting a lot of lines like this one, after "updating environment" step:

updating environment: [new config] 102 added, 0 changed, 0 removed
/home/wo/.cache/pypoetry/virtualenvs/java-driver-xPfC9vS5-py3.11/lib/python3.11/site-packages/recommonmark/parser.py:75: UserWarning: Container node skipped: type=document
  warn("Container node skipped: type={0}".format(mdnode.t))
/home/wo/.cache/pypoetry/virtualenvs/java-driver-xPfC9vS5-py3.11/lib/python3.11/site-packages/recommonmark/parser.py:75: UserWarning: Container node skipped: type=document
  warn("Container node skipped: type={0}".format(mdnode.t))

Visiting the 127.0.0.1:5500 directs me to http://127.0.0.1:5500/master which has 404 error. However pages like http://127.0.0.1:5500/stable/index.html or http://127.0.0.1:5500/scylla-4.15.0.x/ remain functional with redirects to javadoc when clicking on API Documentation header too.

Branchest/Tags need an update: latest tag is 4.18.0.1

could you be more specific? which files shall i update?

I was guessing that we'd need to just add '4.18.0.1' to TAGS in docs/source/conf.py but that actually breaks the build with multiple warnings like:

/tmp/tmpk4aq9ak4/970452ef5b72d76b4f783335fccabe57107d9735/docs/source/manual/core/pooling/index.md: WARNING: document isn't included in any toctree

So after all let's not do that for now.

to silence the warning from sphinx, as we have two "Dropwizard
Metrics" sections in manual/core/metrics/index.md:

```
/home/kefu/dev/scylla-java-driver/docs/_source/manual/core/metrics/index.md:291: WARNING: duplicate label manual/core/metrics/index:dropwizard metrics, other instance in /home/kefu/dev/scyll
a-java-driver/docs/_source/manual/core/metrics/index.md
```

and the warning breaks the "make test". so let's suppress the warning
at this moment.

Signed-off-by: Kefu Chai <[email protected]>
it should start with three backticks not four of them. otherwise
sphinx warns like:

```
/home/kefu/dev/scylla-java-driver/docs/_source/manual/mapper/daos/getentity/index.md:111: WARNING: Lexing literal_block '@GetEntity\nProduct asProduct(Row row);\n\n@GetEntity\nProduct firstR
owAsProduct(ResultSet resultSet);\n```\n\n' as "java" resulted in an error at token: '`'. Retrying in relaxed mode.
```

in this change, let's use three backticks. and the warning disappears.

Signed-off-by: Kefu Chai <[email protected]>
@tchaikov
Copy link
Author

tchaikov commented Jul 13, 2024

make clean preview looks better. Now manual pages can be browsed normally (before they did not work). API Documentation does not link to javadoc.

make clean multiversionpreview does not build correctly for me. For testing I've removed all but 2 versions (4.14.1.x and 4.15.0.x) from BRANCHES in source/conf.py to speed up the build. I'm getting a 404 error page instead. Throughout the build I'm getting a lot of lines like this one, after "updating environment" step:

i commented out "multiversionpreview" to workaround a dependency issue on python 3.12, but forgot to revert it. the latest version dropped this workaround. so the multiversion should be back now.

updating environment: [new config] 102 added, 0 changed, 0 removed
/home/wo/.cache/pypoetry/virtualenvs/java-driver-xPfC9vS5-py3.11/lib/python3.11/site-packages/recommonmark/parser.py:75: UserWarning: Container node skipped: type=document
  warn("Container node skipped: type={0}".format(mdnode.t))
/home/wo/.cache/pypoetry/virtualenvs/java-driver-xPfC9vS5-py3.11/lib/python3.11/site-packages/recommonmark/parser.py:75: UserWarning: Container node skipped: type=document
  warn("Container node skipped: type={0}".format(mdnode.t))

we have the same error on 3.x branch.

Visiting the 127.0.0.1:5500 directs me to http://127.0.0.1:5500/master which has 404 error. However pages like http://127.0.0.1:5500/stable/index.html or http://127.0.0.1:5500/scylla-4.15.0.x/ remain functional with redirects to javadoc when clicking on API Documentation header too.

i think this is due to the removed multiversion in previous reversion. and should be fixed.

Branchest/Tags need an update: latest tag is 4.18.0.1

could you be more specific? which files shall i update?

I was guessing that we'd need to just add '4.18.0.1' to TAGS in docs/source/conf.py but that actually breaks the build with multiple warnings like:

yeah, all 4.x tags are broken at this moment. i think the first step is to fix the 4.x branch.

/tmp/tmpk4aq9ak4/970452ef5b72d76b4f783335fccabe57107d9735/docs/source/manual/core/pooling/index.md: WARNING: document isn't included in any toctree

So after all let's not do that for now.

@tchaikov
Copy link
Author

v3:

  • revert the change to remove multiversion support.

@tchaikov
Copy link
Author

@Bouncheck i see you are still asking for changes. may i learn what exactly the changes i need to make?

@Bouncheck
Copy link
Collaborator

Bouncheck commented Jul 16, 2024

I'm not getting redirected to javadoc when clicking on API Documentation on generated page (make clean preview).
But if that's intended for now I can approve anyway since this PR is a big improvement.
I haven't checked this on 3.x branch but if it's the same there it would make sense to me to approve now and have a separate fix for 3.x + port to 4.x later on.

@tchaikov
Copy link
Author

tchaikov commented Jul 16, 2024

I'm not getting redirected to javadoc when clicking on API Documentation on generated page (make clean preview).

@Bouncheck to my best knowledge, this is indeed intended. see

def redirect_api_page_to_javadoc(app, exception):
version_name = os.getenv("SPHINX_MULTIVERSION_NAME", "")
version_name = "/" + version_name if version_name else ""
redirect_to = version_name +'/api/index.html'
out_file = Path(app.outdir) / 'api.html'
redirects_cli.create(redirect_to=redirect_to, out_file=str(out_file))

we redirect the api doc to ${version_name}/api/index.html, and if version_name is 14.5.6, then the link points to /14.5.6/api/index.html, right under the root directory. i think this should work if the html is deployed to a web server.

But if that's intended for now I can approve anyway since this PR is a big improvement. I haven't checked this on 3.x branch but if it's the same there it would make sense to me to approve now and have a separate fix for 3.x + port to 4.x later on.

i'd appreciate if you could help approve and merge this change. based on this change, we would be able to improve the 4.x document further.

@Bouncheck Bouncheck merged commit 6d7734b into scylladb:scylla-4.x Jul 24, 2024
10 of 12 checks passed
@tchaikov tchaikov deleted the 4.x-docs-backports branch July 24, 2024 13:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants