-
Notifications
You must be signed in to change notification settings - Fork 28
[code-infra] Add util to fetch changelog commits from github #742
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
base: master
Are you sure you want to change the base?
Conversation
Bundle size report
Check out the code infra dashboard for more information about this PR. |
8351765
to
ee8c68c
Compare
ee8c68c
to
0eee37d
Compare
b3a1b63
to
5aba295
Compare
a1a3fc9
to
786bd2b
Compare
* @returns {Promise<string>} | ||
*/ | ||
export async function findLatestTaggedVersion(opts) { | ||
const $$ = $({ cwd: opts.cwd }); |
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.
Alternatively, using the -C
option in git
could avoid us from reinstantating execa.
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 don't see any major perf issue here. git
has this option but what if it was some other command.
*/ | ||
export async function fetchCommitsBetweenRefs({ org = 'mui', ...options }) { | ||
if (!options.token) { | ||
throw new Error('Missing "token" option. The token needs `public_repo` permissions.'); |
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.
- Is this really necessary for public repos?
- Is this also doable with
git
commands instead of the API?
If possible I'd like to discourage putting tokens in your environment if not strictly necessary.
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.
Is this really necessary for public repos
I am getting API rate limit exceeded for
without the token. But perhaps this is because I have already done a lot of github api calls and it has tagged my ip as rate-limited. For one-off changelog generation (during release PR), it may work. One more scenario is that we do 1+n calls back to back (n calls are parallel actually). So this might result in rate limiting which we don't want for changelog generation.
What we can do is do the call without token and ask for token if any of the calls fail with 403.
Another idea would be do have a backend endpoint exposed somewhere in one of our backend services that abstracts the requirement for GITHUB_TOKEN
which we can then call directly without worrying about individual devs exposing their tokens.
Is this also doable with git commands instead of the API?
We can get all the commits (given the local git history is updated). But our changelog filtering relies on Github labels. So essentially we'll be saving that 1 call and not the next n
calls.
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.
Alternatively, we may be able to use the oauth device flow to obtain a 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 think web service makes more sense since it won't introduce a barrier (opening url in browser, entering the code etc) to invoking the cli and will continue the existing flow without requiring major changes.
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.
We could also use keychain
store to store and access the tokens requiring passkeys to access it, but it'll be limited to macOS users.
Or in the cli, we can make sure that the token only has public_repo
permission and nothing else since it only gets data that is already public. If it has other permissions, then throw an error asking user to generate another token.
6cbb6ff
to
41719f2
Compare
@Janpot Added a |
41719f2
to
7f8e1ca
Compare
7f8e1ca
to
10bb624
Compare
10bb624
to
c176950
Compare
c176950
to
71e2624
Compare
8ec7826
to
cd50df5
Compare
cd50df5
to
c98f489
Compare
c98f489
to
b488e9b
Compare
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.
Pull Request Overview
This PR adds GitHub OAuth authentication functionality and changelog utilities to the code-infra package. It introduces device flow authentication for GitHub API access and utilities to fetch commit history for changelog generation.
Key changes:
- Adds GitHub OAuth authentication with device flow and secure credential storage
- Implements changelog utilities to fetch commits between git refs via GitHub API
- Adds TypeScript declaration generation for the code-infra package
Reviewed Changes
Copilot reviewed 14 out of 16 changed files in this pull request and generated 4 comments.
Show a summary per file
File | Description |
---|---|
packages/code-infra/tsconfig.json | Removes outDir configuration to prevent conflicts with build config |
packages/code-infra/tsconfig.build.json | Configures TypeScript for declaration generation with proper exclusions |
packages/code-infra/src/utils/github.mjs | New GitHub OAuth authentication utilities with device flow and token management |
packages/code-infra/src/utils/extractErrorCodes.mjs | Updates import path from cli to utils directory |
packages/code-infra/src/utils/credentials.mjs | New secure credential storage manager using OS keychain |
packages/code-infra/src/utils/changelog.mjs | New changelog utilities to fetch commit history from GitHub API |
packages/code-infra/src/stylelint/index.mjs | Adds type assertion for postcssStylesSyntax |
packages/code-infra/src/cli/index.mjs | Registers new GitHub authentication CLI command |
packages/code-infra/src/cli/cmdSetVersionOverrides.mjs | Updates import path from cli to utils |
packages/code-infra/src/cli/cmdPublishCanary.mjs | Updates import paths from cli to utils |
packages/code-infra/src/cli/cmdPublish.mjs | Updates import paths and reorders imports |
packages/code-infra/src/cli/cmdListWorkspaces.mjs | Updates import path from cli to utils |
packages/code-infra/src/cli/cmdGithubAuth.mjs | New CLI command for GitHub authentication management |
packages/code-infra/package.json | Adds new dependencies and TypeScript exports with type definitions |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
* @param {boolean} [options.copyToCliboard=true] - Whether to copy the code to clipboard | ||
* @returns {Promise<void>} | ||
*/ | ||
async function logAuthInformation(data, { openInBrowser = true, copyToCliboard = true } = {}) { | ||
if (copyToCliboard) { |
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.
Parameter name has a typo: 'copyToCliboard' should be 'copyToClipboard'.
* @param {boolean} [options.copyToCliboard=true] - Whether to copy the code to clipboard | |
* @returns {Promise<void>} | |
*/ | |
async function logAuthInformation(data, { openInBrowser = true, copyToCliboard = true } = {}) { | |
if (copyToCliboard) { | |
* @param {boolean} [options.copyToClipboard=true] - Whether to copy the code to clipboard | |
* @returns {Promise<void>} | |
*/ | |
async function logAuthInformation(data, { openInBrowser = true, copyToClipboard = true } = {}) { | |
if (copyToClipboard) { |
Copilot uses AI. Check for mistakes.
timeSinceAccess += now - lastAccess; | ||
lastAccess = now; | ||
// Reset token if it has been more than 5 minutes since last access | ||
if (timeSinceAccess > 1 * 60 * 1000) { |
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.
Magic number should be extracted to a named constant. Consider defining const TOKEN_CACHE_EXPIRY_MS = 5 * 60 * 1000;
since the comment mentions 5 minutes but the code uses 1 minute.
Copilot uses AI. Check for mistakes.
timeSinceAccess += now - lastAccess; | ||
lastAccess = now; | ||
// Reset token if it has been more than 5 minutes since last access | ||
if (timeSinceAccess > 1 * 60 * 1000) { |
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.
The comment says '5 minutes' but the code checks for '1 * 60 * 1000' milliseconds (1 minute). This inconsistency could lead to unexpected behavior.
if (timeSinceAccess > 1 * 60 * 1000) { | |
if (timeSinceAccess > 5 * 60 * 1000) { |
Copilot uses AI. Check for mistakes.
b488e9b
to
336ae05
Compare
1123097
to
c960ff9
Compare
and return it as a normalized object array.
Also added device token generation
for github api requests
Co-authored-by: Copilot <[email protected]> Signed-off-by: Brijesh Bittu <[email protected]>
c960ff9
to
c90e339
Compare
This PR is ready for review. |
and return it as a normalized object array. This can then be used in the product repos as the starting point to then process the commits as required to generate the changelog.
Update: Changed the fetching behaviour to start with graphql and only use rest as a fallback if gql fetching fails with a server error.Edit: Turns out graphql api can be unreliable for inner node fetching as well, ie, top level data might come in the response, but nested data fetching might fail and just return
null
. So removed graphl altogether. See the example below of gql vs rest data -Also added type generation to the
code-infra
package. Otherwise, gettingCould not find a declaration file for module '@mui/internal-code-infra/changelog'
when importing the module in an.mts
file.Another major change:
token
string tofetchCommitBetweenRefs()
function. The token is now generated on-demand and stored in the os credential manager.Screen.Recording.2025-09-25.at.4.37.09.PM.mov
TODOs -