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

Add documentation build and links checking in CI. #1145

Closed
wants to merge 18 commits into from

Conversation

uggla
Copy link
Contributor

@uggla uggla commented Aug 20, 2020

Sanity check:

  • Have you checked to ensure there aren't other open Pull Requests for the same update/change?

Code changes

(Delete or ignore this section for documentation changes)

  • Are you doing the PR on the next branch?

If the change is a new feature or adding to/changing an existing one:

  • Have you created/updated the relevant documentation page(s)?

Explanations:

@Keats, you told me in a comment that it was something you wanted to add to the CI. I did it as I could handle it and let you manage more complex stuffs.

  • Add zola build and zola check to ensure documentation can be built and does not contain invalid links.
  • Fix links in the current documentation that were broken.
    Maybe I would have to open this PR on the master branch ? Let me know if you would rather this PR on the master branch.

@uggla uggla force-pushed the feature/add_doc_test branch 3 times, most recently from 45ee2dc to b2b19f1 Compare August 21, 2020 13:14
@uggla
Copy link
Contributor Author

uggla commented Aug 21, 2020

Hum, seems like this is suffering from #1056. I think I'm gonna add a couple of retries to try to avoid link checking issue. Maybe not the best solution but better than nothing.

- script: |
cd docs && ..\target\debug\zola check
condition: eq( variables['Agent.OS'], 'Windows_NT' )
displayName: Check documentation links windows
Copy link
Collaborator

Choose a reason for hiding this comment

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

That's no good, it would block the release of Zola if there is a dead link. I would be tempted to move zola check to GH actions actually.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hello @Keats, in fact that was the intention. 😄
To not pass the CI process if for any reason the documentation was broken or of boken links was introduced (Like the ones I corrected). Also it would check this on a regular basis and avoid documentation to deviate and keep it completely updated.
I like to put all checks in the CI, but if you think it annoying I can remove them from this PR.

Copy link
Collaborator

Choose a reason for hiding this comment

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

It's a good instinct but the link checker is not robust yet so that could cause me to comment out these lines in anger if I want to do a release and it's failing for whatever reason

Copy link
Contributor Author

@uggla uggla Aug 31, 2020

Choose a reason for hiding this comment

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

So I comment out the link check part.
I introduced nb_threads_per_cpu to configure the link_checker number of threads. As reducing the number of threads looks more stable at least on AzureDevOps. Maybe because there is some kind of limitation to traffic and especially number of simultaneous connections outgoing from AzureDevOps to avoid DDOS.

# skip_anchor_prefixes = [
# "https://caniuse.com/",
# ]

Copy link
Collaborator

Choose a reason for hiding this comment

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

what's the reason for that change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In fact I did a configuration documentation copy / paste.

[link_checker]
# Skip link checking for external URLs that start with these prefixes
skip_prefixes = [
    "http://127.0.0.1",
]

# Skip anchor checking for external URLs that start with these prefixes
# skip_anchor_prefixes = [
#     "https://caniuse.com/",
# ]

I wanted to avoid broken links errors with http://127.0.0.1 (there are a several errors in the documentation), but so far I don't have to use the "anchors skip part". So I commented it out.

- Ensure documentation can be built.
- Check documentation links are valid.
- Try to see if it is more stable with less thread especially on systems
  with few ressources.
- nb_threads_per_cpu define the number of thread per cpu the
  link_checker will use to check links.
  Example : if your system is 2 cpus and nb_threads_per_cpu = 6 you will
  get 6 * 2 = 12 threads to check links.
- nb_threads_per_cpu default to 4.
@uggla uggla changed the title WIP:Add documentation build and links checking in CI. Add documentation build and links checking in CI. Aug 31, 2020
@uggla uggla requested a review from Keats August 31, 2020 22:19
Copy link
Collaborator

@Keats Keats left a comment

Choose a reason for hiding this comment

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

Overall I like the build on CI but not sure about the rest. We can definitely lower the number of threads for the link checker though, in a more static way for now.

- script: |
cd docs && ..\target\debug\zola build
condition: eq( variables['Agent.OS'], 'Windows_NT' )
displayName: Check documentation build windows
Copy link
Collaborator

Choose a reason for hiding this comment

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

I like that!

@@ -7,10 +7,15 @@ pub struct LinkChecker {
pub skip_prefixes: Vec<String>,
/// Skip anchor checking for these URL prefixes
pub skip_anchor_prefixes: Vec<String>,
pub nb_threads_per_cpu: usize,
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think this should be configurable. Let's keep it at 4 for now and we'll see for configuration later on.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So I come back to the static version

"Number of cpus: {}, using {} threads",
num_cpus::get(),
num_cpus::get() * nb_threads_per_cpu
);
Copy link
Collaborator

Choose a reason for hiding this comment

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

we don't want to print that

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have removed it.


This theme places content first by tucking away navigation in a hidden drawer.

* Built for [Zola](https://www.getzola.com)
* Built for [Zola](https://www.getzola.org)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Those are generated by the https://github.com/getzola/themes repo, fixing it here will not prevent more bad permalinks on the next update.

@uggla uggla requested a review from Keats September 2, 2020 21:43
@Keats Keats closed this Sep 4, 2020
@Keats Keats reopened this Sep 4, 2020
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.

2 participants