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

Reconfigure doc-tests CI build to use new docs image #13457

Closed
tcsc opened this issue Jun 14, 2022 · 7 comments
Closed

Reconfigure doc-tests CI build to use new docs image #13457

tcsc opened this issue Jun 14, 2022 · 7 comments
Assignees
Labels
feature-request Used for new features in Teleport, improvements to current should be #enhancements

Comments

@tcsc
Copy link
Contributor

tcsc commented Jun 14, 2022

The doc-tests build is currently using the outdated next image for doc tests. This should be updated to use an image produced from the new gravitational/docs repo.

Unfortunately, there is no obvious procedure for creating an image from the docs repo, nor one from next that we can copy. I therefore propose the following plan:

  1. Create a GCB trigger in docs that fires on a merge to master that rebuilds and pushes a docs image to quay.io
  2. re-target the doc-tests build in teleport to use new docs image
  3. fix any failures in the doc tests caused by (2)

Open questions:

  • Is quay.io still the best place to push that image? Should we push it to the Google registry as it will be both built and consumed in the Google cloud?
  • Has anyone tested the Dockerfile in docs? Is it expected to work and/or contain all the machinery required for the Teleport doc-tests build?

This is motivated by a discrepancy between teleport docs liniting and docs docs linting that started occurring after gravitational/docs#50.

@tcsc tcsc added the feature-request Used for new features in Teleport, improvements to current should be #enhancements label Jun 14, 2022
@tcsc tcsc self-assigned this Jun 14, 2022
@wadells
Copy link
Contributor

wadells commented Jun 16, 2022

cc @logand22 for reflections on the following in light of #13165

Is quay.io still the best place to push that image? Should we push it to the Google registry as it will be both built and consumed in the Google cloud?

@wadells
Copy link
Contributor

wadells commented Jun 16, 2022

Has anyone tested the Dockerfile in docs? Is it expected to work and/or contain all the machinery required for the Teleport doc-tests build?

@ptgott is probably the best to address this, since he originally reported the discrepancy between docs CI and what Teleport runs. AFAICT, the docs Dockerfile shares history and format with the next Dockerfile. Here's the last commit containing the next docs infra:

https://github.com/gravitational/next/blob/65c258de58c90fd3c250b45c2be00e95357daceb/.drone.yml#L30-L32

https://github.com/gravitational/next/blob/65c258de58c90fd3c250b45c2be00e95357daceb/Makefile#L1-L16

https://github.com/gravitational/next/blob/65c258de58c90fd3c250b45c2be00e95357daceb/Dockerfile#L1-L5

So you should 🤞 be good to go.


Other discussion:

Here are the checks that the docs repo itself currently runs:

https://github.com/gravitational/docs/blob/b59f285f817f16c6391ba2bc100aaa6210e1cc5f/.github/workflows/base.yml#L14-L21

I know this is a bit different than what we run in teleport:

- ln -s /workspace /src/content && yarn markdown-lint

As long as we're looking at this, I want to double check both these look correct. E.g. would a yarn markdown-lint on gravitational/docs#50 have caught the issue when the "breaking" changes were being made?

@logand22
Copy link
Contributor

Is quay.io still the best place to push that image? Should we push it to the Google registry as it will be both built and consumed in the Google cloud?

gravitational/docs is public, so I expect that external contributors would need access to the image if they want to test locally. If the image is public I recommend we stay consistent with the registry we use. In this case the current public image registry we use is quay.io with #13165 replacing that in the future.

@ptgott
Copy link
Contributor

ptgott commented Jun 16, 2022

@wadells Related to the checks in gravitational/docs, the yarn typecheck, yarn lint-check, and yarn test commands perform checks on the JavaScript/TypeScript files involved in building the NextJS site, e.g., our remark plugins and React components. These checks don't encompass docs changes to gravitational/teleport.

markdown-lint checks individual pages within the docs site. gravitational/docs, like gravitational/next, includes a git submodule for each currently supported version branch of gravitational/teleport (e.g., master, branch/v10, branch/v9, and branch/v8). Since the docs are included in the gravitational/teleport repo, markdown-lint assumes that git submodule init and git submodule update have been run, populating docs pages in content/<version>. yarn markdown-lint checks all MDX files within each of these submodules.

@wadells
Copy link
Contributor

wadells commented Jun 16, 2022

@ptgott Understood. My question is: Should we run markdown-lint in docs CI? My concern is that toolchain changes that affect the validity of content in the various submodules wouldn't be caught until after they're merged in docs.

More holistically: What is the expected process when a change is made that affects both content in docs and teleport (e.g. a toolchain upgrade)? Who is responsible for fixing new lint errors in the docs?

@ptgott
Copy link
Contributor

ptgott commented Jun 16, 2022

@wadells I don't think we should run markdown-lint in CI for the docs repo, since changing a linter rule in docs could cause markdown-lint to fail, and the Web team isn't responsible for changes to docs pages. markdown-lint is intended to catch issues with changes to the docs in gravitational-teleport.

The last time there was a change that affected both docs and teleport that I can remember was a new linter rule in docs that required Details components to have a title property. A member of the Web team warned me that this change was coming, and I edited all affected Details components to include titles (or changed them to other components).

I think it might help to bring @C-STYR and @jeffgaynor into this conversation as well: do you have a process you'd recommend for making linter rule changes without causing chaos in the teleport CI pipeline?

@ptgott
Copy link
Contributor

ptgott commented Jun 21, 2022

@wadells Here are some affected PRs:

It actually turns out that few in-flight PRs are actively blocked, though as I do more docs reorganization work (per this issue), I'm guessing there will be more.

tcsc added a commit to gravitational/docs that referenced this issue Jun 22, 2022
Adds a build script that will construct and publish the `docs` image to
`quay.io`. This build script will be triggered by Google Cloud Build
on a push to `main`.

The upshot of this script is that the image used by Teleoprt CI to
run its doctests will be automatically updated every time a PR is
merged in this repository.

See-Also: gravitational/teleport#13457
tcsc added a commit that referenced this issue Jun 23, 2022
Updates the image used to run the doc-tests CI build and pulls in changes from #13774 to fix compatibility issues with the new image.

See-Also: #13457
Co-authored-by: Paul Gottschling <[email protected]>
tcsc added a commit to gravitational/docs that referenced this issue Jun 24, 2022
Adds a build script that will construct and publish the `docs` image to
`quay.io`. This build script will be triggered by Google Cloud Build
on a push to `main`.

The upshot of this script is that the image used by Teleoprt CI to
run its doctests will be automatically updated every time a PR is
merged in this repository.

See-Also: gravitational/teleport#13457
@tcsc tcsc closed this as completed Jul 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature-request Used for new features in Teleport, improvements to current should be #enhancements
Projects
None yet
Development

No branches or pull requests

4 participants