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

🌱 book: refactor verification #6159

Merged

Conversation

sbueringer
Copy link
Member

Signed-off-by: Stefan Büringer [email protected]

What this PR does / why we need it:
This PR refactors our book verification/build as described in #6141.

The verify-book-links target never really verified book links in the sense of checking for broken links.

After this PR we have make book-build target which is run on PRs as part of the build job.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Implements part of #6141

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Feb 17, 2022
@sbueringer
Copy link
Member Author

/assign @CecileRobertMichon

@k8s-ci-robot k8s-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Feb 17, 2022
@sbueringer
Copy link
Member Author

/cherry-pick release-1.1
As described on the issue. Let's implement this for main and release-1.1 and keep older branches as is.

@k8s-infra-cherrypick-robot

@sbueringer: once the present PR merges, I will cherry-pick it on top of release-1.1 in a new PR and assign it to you.

In response to this:

/cherry-pick release-1.1
As described on the issue. Let's implement this for main and release-1.1 and keep older branches as is.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@killianmuldoon
Copy link
Contributor

For some reason this doesn't seem to work for me with make -C docs/book build but it's throwing errors when I run the link checker in standalone mode. Any ideas why? (The older verify target has the same behaviour)

@sbueringer
Copy link
Member Author

sbueringer commented Feb 17, 2022

For some reason this doesn't seem to work for me with make -C docs/book build but it's throwing errors when I run the link checker in standalone mode. Any ideas why? (The older verify target has the same behaviour)

What are you running, in which sense doesn't it work?
What do you mean with link checker in standalone mode?

This PR drops the verify target because it never really was a link checker, it just built the book :)

Our actual link checker is run as GitHub action (lint-docs.yaml)

@killianmuldoon
Copy link
Contributor

Right - that's what I was missing. I can run ./hack/tools/bin/mdbook-linkcheck --standalone docs/book/ locally to check the links in the book.

Copy link
Contributor

@killianmuldoon killianmuldoon left a comment

Choose a reason for hiding this comment

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

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 17, 2022
@sbueringer
Copy link
Member Author

Right - that's what I was missing. I can run ./hack/tools/bin/mdbook-linkcheck --standalone docs/book/ locally to check the links in the book.

A nice info, another linkchecker, I wasn't aware of that one :)

@CecileRobertMichon What do you think, should we keep that one or remove it too?

/hold
for a bit

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 17, 2022
@killianmuldoon
Copy link
Contributor

This one is installed in that make target I'm pretty sure

@sonasingh46
Copy link

Here is one thought:

  • Make a target that actually checks for book links.
  • Use that target itself in the CI.

@sbueringer
Copy link
Member Author

sbueringer commented Feb 17, 2022

Here is one thought:

  • Make a target that actually checks for book links.
  • Use that target itself in the CI.

In general +1. It just seems like the GitHub action we have works pretty nicely. So I'm not sure if we should built a target which does the same as the GithubAction and then run make as a GithubAction.

Maybe we should keep the GitHub action as is and add a target which does the same as the action? (tl;dr I'm not sure if it's worth enforcing to run make in CI as GitHub actions are usually nicely integrated with GitHub, e.g. see the nicely minimal GitHub action config)

@sonasingh46
Copy link

+1 @sbueringer

Signed-off-by: Stefan Büringer [email protected]
@sbueringer sbueringer force-pushed the pr-refactor-book-verification branch from 89c1809 to 7cdce7c Compare February 22, 2022 07:42
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 22, 2022
@sbueringer
Copy link
Member Author

Dropped the other linkchecker too.

Ashutosh and I talked somewhere in Slack, the idea would be for now to only have the GitHub action as linkchecker. If there is demand we can always add a new make target to make it locally runnable. But up until now that wasn't really necessary.

@sbueringer
Copy link
Member Author

/cc @fabriziopandini

Copy link
Member

@vincepri vincepri left a comment

Choose a reason for hiding this comment

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

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 23, 2022
Copy link
Member

@fabriziopandini fabriziopandini left a comment

Choose a reason for hiding this comment

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

/approve
/lgtm

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: fabriziopandini

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 24, 2022
@sbueringer
Copy link
Member Author

/hold cancel

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 24, 2022
@k8s-ci-robot k8s-ci-robot merged commit 61e7dcc into kubernetes-sigs:main Feb 24, 2022
@k8s-ci-robot k8s-ci-robot added this to the v1.2 milestone Feb 24, 2022
@k8s-infra-cherrypick-robot

@sbueringer: #6159 failed to apply on top of branch "release-1.1":

Applying: book: refactor verification
Using index info to reconstruct a base tree...
M	Makefile
Falling back to patching base and 3-way merge...
Auto-merging Makefile
CONFLICT (content): Merge conflict in Makefile
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 book: refactor verification
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

In response to this:

/cherry-pick release-1.1
As described on the issue. Let's implement this for main and release-1.1 and keep older branches as is.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@sbueringer
Copy link
Member Author

manual cherry-pick: #6204

@sbueringer sbueringer deleted the pr-refactor-book-verification branch February 24, 2022 15:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants