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

get latest redirects based on nav-data not version metadata #2563

Merged
merged 5 commits into from
Sep 13, 2024

Conversation

zchsh
Copy link
Contributor

@zchsh zchsh commented Sep 9, 2024

πŸ”— Relevant links

πŸ—’οΈ What

This PR avoids using the ref from version-metadata in order to fetch the "latest" redirects from content source repositories.

🀷 Why

This work is in service of [DEVDOT-???] Release branches as docs single source of truth. At present, our reliance on version-metadata is out of convenience, and assumes that version-metadata will have its ref and sha updated on every content upload, even when there have been zero changes to version metadata. This method of constant updates seems to be causing issues that outweigh the convenience of this approach.

πŸ› οΈ How

This PR refactors getLatestContentRefForProduct, which upstream fetches the latest ref from version-metadata, into getLatestContentShaForProduct, which now fetches the latest sha from known nav-data endpoints.

πŸ§ͺ Testing

Tests have been added to assert that redirects are successfully fetched with this approach. In addition, we have a more ephemeral demo, formatted as a test file for convenience, that asserts that the redirects fetched using the sha from nav-data are identical to redirects fetched using the ref from version-metadata:

/**
 * OVERVIEW
 *
 * Intent here is to demo that redirects fetched using the latest SHA from
 * content match those fetched with refs from the latest version metadata.
 *
 * Note that as we make changes to how version metadata is updated, we expect
 * this demo will no longer work. This is intentional, as version metadata will
 * start reflecting the `ref` and `sha` at the last point the version metadata
 * was updated, which will often fall behind the `sha` of the latest content
 * upload.
 *
 * HOW TO USE THIS FILE
 *
 * Drop this file in `build-libs/__tests__/latest-content-sha-demo.test.ts`,
 * in the `dev-portal` repo, then run `npm run test -- latest-content-sha-demo`.
 */

import getLatestContentShaForProduct from '../get-latest-content-sha-for-product'
import fetchGithubFile from '@build-libs/fetch-github-file'
import { PRODUCT_REDIRECT_ENTRIES } from '@build-libs/redirects'

/**
 * Mirror the previous implementation of the function, which fetched
 * version metadata. We'll use this to compare the new `sha` approach with
 * the existing `sha` values that correspond to each `ref`.
 */
async function getLatestVersionMetadata(product) {
	const contentUrl = new URL('https://content.hashicorp.com')
	contentUrl.pathname = `/api/content/${product}/version-metadata/latest`
	return await fetch(contentUrl.toString()).then((res) => res.json())
}

/**
 * Run demo "tests"
 */
describe('latestContentShaDemo', () => {
	PRODUCT_REDIRECT_ENTRIES.forEach(({ repo, path }) => {
		/**
		 * Demo that the SHA matches between nav-data and version metadata.
		 * This is mostly what we care about, we're asserting that the `sha`
		 * in `nav-data` references an equivalent point in git history as the
		 * `ref` in version metadata.
		 */
		it(`demos that the latest SHA for nav-data in "${repo}" matches the SHA in the latest version metadata`, async () => {
			const latestSha = await getLatestContentShaForProduct(repo)
			const latestVersionMetadata = await getLatestVersionMetadata(repo)
			expect(latestSha).toBe(latestVersionMetadata.result.sha)
		})

		/**
		 * Demo that the redirects file is identical, which is mostly asserting
		 * that `fetchGithubFile` can work with a `ref` or `sha`. We skip
		 * private repos, as fetching that data would require authentication.
		 */
		if (['hcp-docs', 'sentinel', 'ptfe-releases'].includes(repo)) {
			console.log(`Skipping test for "${repo}" repo, as it's a private repo.`)
		} else {
			it(`demos that the redirects fetched from "${repo}" using the latest version metadata ref match redirects fetched from "${repo}" using the latest nav-data SHA`, async () => {
				// Previous approach, using version metadata
				const latestVersionMetadata = await getLatestVersionMetadata(repo)
				const redirectsFromRef = await fetchGithubFile({
					owner: 'hashicorp',
					repo: repo,
					path: path,
					ref: latestVersionMetadata.result.ref,
				})
				// New approach, using SHA from nav-data
				const latestSha = await getLatestContentShaForProduct(repo)
				const redirectsFromSha = await fetchGithubFile({
					owner: 'hashicorp',
					repo: repo,
					path: path,
					ref: latestSha,
				})
				expect(redirectsFromRef).toBe(redirectsFromSha)
			})
		}
	})
})

πŸ’­ Anything else?

Not at the moment!

Copy link

vercel bot commented Sep 9, 2024

The latest updates on your projects. Learn more about Vercel for Git β†—οΈŽ

Name Status Preview Comments Updated (UTC)
dev-portal βœ… Ready (Inspect) Visit Preview πŸ’¬ Add feedback Sep 13, 2024 5:40pm

Copy link

github-actions bot commented Sep 9, 2024

πŸ“¦ Next.js Bundle Analysis

This analysis was generated by the next.js bundle analysis action πŸ€–

This PR introduced no changes to the javascript bundle πŸ™Œ

@zchsh zchsh marked this pull request as ready for review September 9, 2024 19:01
@zchsh zchsh changed the title get latest ref from nav-data not version metadata get redirects from nav-data not version metadata Sep 9, 2024
@zchsh zchsh changed the title get redirects from nav-data not version metadata get latest redirects based on nav-data not version metadata Sep 9, 2024
Copy link
Contributor

Choose a reason for hiding this comment

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

I may be overlooking something here, but why is this a .ts file while the other two are .js?

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 think we'd prefer to use TS in both places... but we import the redirects stuff into next.config.js, so it needs to be JS rather than TS. Tests can be TS since they don't need to run directly in that context πŸ‘

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah! Yeah that makes sense.

Copy link
Contributor

@rmainwork rmainwork left a comment

Choose a reason for hiding this comment

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

LGTM πŸš€

Not going to hold the review up over that comment - was just curious.

Copy link
Collaborator

@heatlikeheatwave heatlikeheatwave left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Collaborator

@RubenSandwich RubenSandwich left a comment

Choose a reason for hiding this comment

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

I agree with this change, we might as well go to the source of truth rather than having a redirects in our logic.

@zchsh zchsh merged commit a612e0b into main Sep 13, 2024
9 checks passed
@zchsh zchsh deleted the zs.ref-for-redirects branch September 13, 2024 17:50
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.

5 participants