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

[v8] Fix Doctests CI (#10117) #10149

Merged
merged 9 commits into from
Feb 9, 2022
Merged

[v8] Fix Doctests CI (#10117) #10149

merged 9 commits into from
Feb 9, 2022

Conversation

tcsc
Copy link
Contributor

@tcsc tcsc commented Feb 4, 2022

The linting applied to file via the tools in next appears to vary if the files
are outside the /src directory: If the files are under /src, a rigorous lint
is applied. If the files are outside of /src, a "less-rigorous" lint is
applied, letting many legitimate issues slip through.

This patch alters the CI script to symlink the /workspace directory (the
mount-point that GCB uses to inject code into the container running a build
step) under the next image's /src/content directory, so that the correct,
rigorous lint will be applied.

It also fixes the lint errors that have crept in during the time the linter
was being incorrectly lenient.

See-Also: #9600
See-Also: #10107

Backports #10117

The linting applied to file via the tools in next appears to vary if the files
are outside the /src directory: If the files are under /src, a rigorous lint
is applied. If the files are outside of /src, a "less-rigorous" lint is
applied, letting many legitimate issues slip through.

This patch alters the CI script to symlink the /workspace directory (the
mount-point that GCB uses to inject code into the container running a build
step) under the next image's /src/content directory, so that the correct,
rigorous lint will be applied.

It also fixes the lint errors that have crept in during the time the linter
was being incorrectly lenient.

See-Also: #9600
See-Also: #10107
@tcsc tcsc changed the title Fix Doctests CI (#10117) [v8] Fix Doctests CI (#10117) Feb 4, 2022
@tcsc tcsc enabled auto-merge (squash) February 4, 2022 07:18
@zmb3
Copy link
Collaborator

zmb3 commented Feb 4, 2022

@tcsc what PR is this the backport of?

args: ['yarn', 'remark', '/workspace/docs/pages/**/*.mdx', '--frail']
args:
- -c
- ln -s /workspace /src/content && yarn markdown-lint-external-links
Copy link
Collaborator

Choose a reason for hiding this comment

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

Commenting here because I got added as a reviewer and I don't see a link to the original PR.

I don't love that this creates behavior that differs locally versus when run on GCB. While I'm fine with this as a temporary fix, it would be nice to create a follow up issue to investigate a better fix.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We're at the mercy of to conflicting restrictions here:

  1. We can't use make -C build.assets test-docs on a buildbox image as you would locally, as this needs Docker-in-Docker for CI, which we don't want to allow, and
  2. There is also no way we can customize the mount point that GCB uses (it is always /workspace), so we can't change that to move the docs source into place.

The actual test-docs task looks like this:

DOCSBOX=quay.io/gravitational/next:main

test-docs: docsbox
	docker run --platform=linux/amd64 -i $(NOROOT) -v $$(pwd)/..:/src/content $(DOCSBOX) \
		/bin/sh -c "yarn markdown-lint-external-links"

...so the build steps in the CI build file are the closest equivalent I could make that would work under GCB

@tcsc
Copy link
Contributor Author

tcsc commented Feb 6, 2022

@tcsc what PR is this the backport of?

#10117

@tcsc tcsc merged commit 2be57a8 into branch/v8 Feb 9, 2022
@tcsc tcsc deleted the tcsc/backport-v8/doc-tests branch February 9, 2022 03:27
@webvictim webvictim mentioned this pull request Mar 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants