-
Notifications
You must be signed in to change notification settings - Fork 0
Add helper to fetch SDK artifacts directly from Github #190
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
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
2077026 to
539a7a5
Compare
| const sdkConfigs = await withGithubSdks( | ||
| [ | ||
| { | ||
| language: "typescript", | ||
| owner: "gleanwork", | ||
| repo: "api-client-typescript", | ||
| version: "v0.11.2", | ||
| tryItNow: { | ||
| outDir: "./public/glean-try-it-now", | ||
| urlPrefix: "/glean-try-it-now", | ||
| }, | ||
| }, | ||
| { | ||
| language: "curl", | ||
| }, | ||
| ], | ||
| process.env.GITHUB_TOKEN | ||
| ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know we originally specced out this helper to exist at this level, but now that I see it in practice I'm not sure I love it.
What if instead we did something like this?
export default {
spec: "../specs/glean.yaml",
output: {
pageOutDir: "./docs/glean/api",
embedOutDir: "./src/components/glean-embeds",
framework: "docusaurus",
},
display: {
maxNestingLevel: 2,
},
codeSamples: [
withGitHubSdk({
language: "typescript",
owner: "gleanwork",
repo: "api-client-typescript",
version: "v0.11.2",
tryItNow: {
outDir: "./public/glean-try-it-now",
urlPrefix: "/glean-try-it-now",
},
})
]
};This way, we allow more flexibility by scoping the GitHub part just to the code sample in question. Who knows, maybe folks might want to use different providers for different languages...if nothing else GitHub definitely doesn't apply to cURL samples.
We could implement this by changing the signature of codeSamples in the main Settings type to be something like CodeSample | (config: unknown) => Promise<CodeSample> or something
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I like this. And accepting config values (like codeSamples in this case) as promises at the main Settings level adds a lot of flexibility for what customers can do too, which I like.
I'll make those updates and we can go from there!
cbc9b94 to
36e00ef
Compare
|
This change is part of the following stack: Change managed by git-spice. |
36e00ef to
c2d1c88
Compare
|
@nebrius I revised the PR to inline the helper. In particular, I was curious about your thoughts on the pattern I am using to unwrap promise values from the config. Could you take another pass when you get the chance? And I'll update the withGithubSpec PR next (it's still just a draft)! Just wanted to get the overall helper pattern nailed down before updating it in the same manner. |
nebrius
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, just one last little thing
| process.exit(1); | ||
| } | ||
| } | ||
| throw new InternalError( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| throw new InternalError( | |
| throw new Error( |
I bet the most likely reason for this will be stuff like not being able to connect to the server, lookup DNS, etc., which could be due to internet issues on the user's end.
In this case, it's not a bug on our part, and so should be a normal error.
This PR adds a helper to allow fetching SDK artifacts directly from Github. This helper works inline with other
codeSampleSDK configs. It takes the standard SDK config with additional parameters to specify repo and version tag and an optional Github token. While I can definitely see use cases where you want a unique token per SDK, this helper also allows defaulting to a token set as the environment variable$GITHUB_TOKENto avoid getting too verbose when that level of configuration isn't needed..It returns a promise to a code sample instance with
sdkTarballPathset to a tarball of the SDK repo downloaded directly from Github. I updated the types so this helper can only be used withcodeSampletypes that require an SDK artifact (which right now just means notcurl).To gracefully handle promises to config values and the underlying values themselves, I am using
zod'spreprocess()function to allow awaiting resolution of the config value before parsing the config withsafeParseAsync(). I went with this approach because when reading the docs, I noticedz.promise()is deprecated in the new v4, and I think unwrapping the value before parsing is cleaner anyway and perfectly well supported bypreprocess()since it supports async pre-processing functions.This PR uses
octokit.jsto download a Tarball archive for the specified SDK repo using the Personal Access Token for auth.