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

ci: Weekly check whether the docs url is alive #2482

Merged
merged 1 commit into from
Mar 21, 2022

Conversation

Bevisy
Copy link
Member

@Bevisy Bevisy commented Aug 23, 2021

Weekly check(at 23:00 every Sunday) whether the docs url is ALIVE, so that
we can find the failed url in time

Fixes #815

Signed-off-by: Binbin Zhang binbin36520@gmail.com

@Bevisy Bevisy changed the title makefile: Schedule daily static checking docs makefile: Schedule static check docs weekly Aug 23, 2021
@devimc
Copy link

devimc commented Aug 23, 2021

thanks @Bevisy
cc @jodh-intel

.github/workflows/static-check-docs.yaml Outdated Show resolved Hide resolved
.github/workflows/static-check-docs.yaml Outdated Show resolved Hide resolved
@Bevisy Bevisy force-pushed the main-815 branch 3 times, most recently from b9c55ca to 5eb44d2 Compare August 31, 2021 08:05
@fidencio
Copy link
Member

@Bevisy, sorry for coming late to the party, but why having those checked on weekly basis and not on every PR?

@Bevisy
Copy link
Member Author

Bevisy commented Aug 31, 2021

@Bevisy, sorry for coming late to the party, but why having those checked on weekly basis and not on every PR?

I can't agree more to add docs check to static-checks ci workflow. Periodic examination results are not sufficiently visual.

@Bevisy
Copy link
Member Author

Bevisy commented Aug 31, 2021

I've changed from periodic checks to triggering checks on every pr push and only triggering checks on all *.md documents when they are modified.
The execution time of doc check once for a local test is as follows:

make static-check-docs  5.81s user 2.01s system 2% cpu 5:35.94 total

If pr does not contain document changes, the execution time as follows:

make static-check-docs  0.71s user 0.23s system 10% cpu 9.346 total

It can be concluded that the ci time consumption brought is tolerable.

@Bevisy Bevisy changed the title makefile: Schedule static check docs weekly ci: Add docs check to workflows/static-checks Aug 31, 2021
Copy link
Contributor

@jodh-intel jodh-intel left a comment

Choose a reason for hiding this comment

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

Thanks @Bevisy!

lgtm

It's worth noting that it might still be worth running this check periodically as well. It's just about possible that we might not get any new PRs for a few days, but if we ran the checks daily, we would detect broken URLs faster. That said, it's more CI time and more complexity so this is fine for now 😄

@jodh-intel
Copy link
Contributor

/test

@Bevisy
Copy link
Member Author

Bevisy commented Sep 6, 2021

Hi, @jodh-intel @fidencio , I found workflow/static-checks always execute docs check if file changed

$ /home/zbb/go/src/github.com/kata-containers/tests/.ci/static-checks.sh src/runtime
[static-checks.sh:1148] INFO: Running 'static_check_commits' function
make -C cmd/checkcommits
make[1]: Entering directory '/home/zbb/go/src/github.com/kata-containers/tests/cmd/checkcommits'
go test .
ok      github.com/kata-containers/tests/cmd/checkcommits       (cached)
go install -ldflags "-X main.appCommit="2b71534dcb403a2341db35ce87a12ff62af793dc" -X main.appVersion=0.0.1" .
make[1]: Leaving directory '/home/zbb/go/src/github.com/kata-containers/tests/cmd/checkcommits'
From github.com:kata-containers/tests
 = [up to date]        main       -> origin/main
Running checkcommits version 0.0.1 (commit 2b71534dcb403a2341db35ce87a12ff62af793dc)
Found 1 commit between commit HEAD and branch main
All commit checks passed.
[static-checks.sh:1148] INFO: Running 'static_check_docs' function
[static-checks.sh:608] INFO: Checking documentation
[static-checks.sh:626] INFO: Checking local branch for changed documents only
[static-checks.sh:663] INFO: Checking document markdown references
...

So it means that adding new step/static-doc-check is a duplicate step. Maybe we should add new workflow to check all docs url periodically just as we expected at the beginning

I will do some fix later 😄

@Bevisy Bevisy force-pushed the main-815 branch 2 times, most recently from 2d054b1 to f4fb9ed Compare September 14, 2021 08:18
@Bevisy Bevisy changed the title ci: Add docs check to workflows/static-checks ci: Weekly check whether the docs url is alive Sep 14, 2021
@Bevisy
Copy link
Member Author

Bevisy commented Sep 14, 2021

I have updated the fix. And when the docks-url-alive-check workflow excuted, we could find there two url dead

ERROR: Invalid URL 'https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/filesystems/dax.txt' found in the following files:

docs/design/architecture.md
ERROR: Invalid URL 'https://projectacrn.github.io/latest/tutorials/kbl-nuc-sdc.html#use-the-script-to-set-up-acrn-automatically' found in the following files:

docs/how-to/how-to-use-kata-containers-with-acrn.md

The dead url should update to

https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/filesystems/dax.rst

https://projectacrn.github.io/2.1/tutorials/kbl-nuc-sdc.html#use-the-script-to-set-up-acrn-automatically

@Bevisy
Copy link
Member Author

Bevisy commented Sep 14, 2021

/test

@jodh-intel
Copy link
Contributor

/test

@jodh-intel
Copy link
Contributor

/test-ubuntu-qat

@jodh-intel
Copy link
Contributor

Any further comments @bergwolf?

@cmaf
Copy link
Contributor

cmaf commented Nov 9, 2021

@Bevisy could you please rebase?

jodh-intel
jodh-intel previously approved these changes Dec 23, 2021
Copy link
Contributor

@jodh-intel jodh-intel left a comment

Choose a reason for hiding this comment

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

Thanks @Bevisy.

lgtm

@jodh-intel
Copy link
Contributor

@fidencio - could you tal at this one?

target_branch: ${{ github.base_ref }}
steps:
- name: Install Go
if: ${{ !contains(github.event.pull_request.labels.*.name, 'force-skip-ci') }}
Copy link
Member

Choose a reason for hiding this comment

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

I see that you are using this check at every step. Why not just check this once as the first step and return early if true.

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

@Bevisy Can you address this so that we can have this PR merged?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm sorry I don't see the syntax for this feature on github action, it looks like the steps can only be executed one at a time in sequence

.github/workflows/docs-url-alive-check.yaml Outdated Show resolved Hide resolved
@jodh-intel
Copy link
Contributor

Ping for a review y'all!

Weekly check(at 23:00 every Sunday) whether the docs url is ALIVE, so that
we can find the failed url in time

Fixes kata-containers#815

Signed-off-by: Binbin Zhang <binbin36520@gmail.com>
@amshinde
Copy link
Member

/test

@Bevisy
Copy link
Member Author

Bevisy commented Jan 24, 2022

Hold on, we can't simply remove the Travis-related environment variables, but need to do further investigation. I will do some cleaning under issue #3544 and then re-update the validation workflow here.

@fidencio
Copy link
Member

A head's up that may affect your PR.

In the Architecture Committee meeting from January 25th, 2022, the Architecture Committee has agreed on using the "Dismiss stale pull request approvals when new commits are pushed" configuration from GitHub. It basically means that if your PR has been rebased or updated, the approvals given will be erased.

In order to minimize traumas due to the new approach, please, consider adding a note on the changes done before the rebase / force-push, and also pinging the reviewers for a subsequent round of reviews.

Thanks for your understanding!

Related issue: kata-containers/community#249

@fidencio fidencio added the do-not-merge PR has problems or depends on another label Feb 1, 2022
@fidencio
Copy link
Member

fidencio commented Feb 1, 2022

Adding the do-not-merge label as per @Bevisy's comment.

Copy link
Contributor

@jodh-intel jodh-intel left a comment

Choose a reason for hiding this comment

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

Thanks @Bevisy!

lgtm

@jodh-intel
Copy link
Contributor

@Bevisy - Could you fix those invalid URL's the checker found on another commit on this PR maybe?

@katacontainersbot katacontainersbot added the size/medium Average sized task label Mar 9, 2022
@jodh-intel
Copy link
Contributor

Weighing up the duplication versus the ability to check all our docs daily, I say let's land this!

inter-step comms and skips are extremely awkward with GHA's, so if that language every grows the ability to do what we want in a simpler way, we can iterate this GHA later imho.

@jodh-intel jodh-intel removed the do-not-merge PR has problems or depends on another label Mar 10, 2022
@jodh-intel
Copy link
Contributor

Ping for more reviews folks...

@jodh-intel
Copy link
Contributor

Can we get one more review here please?

Copy link
Member

@Jakob-Naucke Jakob-Naucke left a comment

Choose a reason for hiding this comment

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

LGTM. OOI where would we see an unsuccessful run? It wouldn't be in the PRs, right?

@Jakob-Naucke Jakob-Naucke added area/testing Issues related to testing no-backport-needed labels Mar 21, 2022
@Bevisy
Copy link
Member Author

Bevisy commented Mar 21, 2022

LGTM. OOI where would we see an unsuccessful run? It wouldn't be in the PRs, right?

We can see the results of the execution in the actions. We don't get email notifications, so we need to check the results of the actions periodically.

@fidencio
Copy link
Member

/test-ppc64le

@fidencio
Copy link
Member

/test-ppc

@fidencio
Copy link
Member

/test-power

@jodh-intel
Copy link
Contributor

To give a bit of context to this PR:

  • The static-checks.sh script currently checks all URLs in all documents that a PR modifies.

  • This PR adds a GHA that would check all URLs in all Kata documents every week.

  • Checking doc URLs is necessary since URLs expire / die / get changed all the time, so to ensure our docs are as up-to-date as possible, we need to check all URLs as regularly as possible (I think we might want to consider daily at some point, but let's keep an eye on this GHA when it lands to see if we need to run it more regularly).

  • In theory, we could make the static-checks.sh script check all Kata documents when a PR is raised (not just those modified by a PR). However, the problem is that in practise we hit the dreaded GitHub API rate limit.
    The solution to that problem is to run the checker in the CI as an authenticated user if someone has free cycles to look at this:

    Use authentication when checking GitHub URLs in static check script tests#4304

@fidencio fidencio merged commit aa6886f into kata-containers:main Mar 21, 2022
@Bevisy Bevisy deleted the main-815 branch June 16, 2022 06:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/testing Issues related to testing size/medium Average sized task
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Create GitHub action to check documentation URLs
9 participants