introduced change that fixes issue #91#128
Conversation
natalialuzuriaga
left a comment
natalialuzuriaga
left a comment
There was a problem hiding this comment.
Before approving, just wanted to ask about the current implementation choices and see whether other alternate approaches could be better suited. I also defer to Sachin though since he wrote the existing code!
| async function calculateMetaData( | ||
| existingCodeJSON?: CodeJSON | null, | ||
| ): Promise<Partial<CodeJSON>> { |
There was a problem hiding this comment.
It is a big change to add existingCodeJSON as an argument to this core function and wondering if this is necessary to do? Based on reviewing the code, I see an existing pattern where merging content between existingCodeJSON and partialCodeJSON occurs in the main.ts (I'll point to it in the next comment). Also, the getBasicInfo() function does very similar functionality as getVersion()where it calls the GitHub API to fetch data about the repo and eventually merges this data with existingCodeJSON
Just wanted to flag and see if we can follow patterns laid out in the existing code for consistency and evaluate whether adding the existingCodeJSON argument is truly necessary
| const partialCodeJSON = await helpers.calculateMetaData(existingCodeJSON); | ||
|
|
||
| // preserve existing feedback mechanisms if they exist, otherwise default to GitHub Issues | ||
| const feedbackMechanism = | ||
| existingCodeJSON?.feedbackMechanism || | ||
| `${partialCodeJSON.repositoryURL}/issues`; | ||
|
|
||
| // preserve existing SBOM link if they exist, otherwise default to GitHub SBOM link | ||
| const SBOM = | ||
| existingCodeJSON?.SBOM || | ||
| `${partialCodeJSON.repositoryURL}/network/dependencies`; |
There was a problem hiding this comment.
Here and below shows the pattern of how we merge existingCodeJSON with partialCodeJSON. We can do the same here with version?
| async function getBasicInfo(): Promise<BasicRepoInfo> { | ||
| try { | ||
| const [repoData, languagesData] = await Promise.all([ | ||
| octokit.rest.repos.get({ owner, repo }), | ||
| octokit.rest.repos.listLanguages({ owner, repo }), | ||
| ]); |
There was a problem hiding this comment.
Offering another implementation approach: we could instead add the version functionality to getBasicInfo() since it also uses octokit. Does octokit.rest.repos.get({owner, repo}) receive data on the repo's release tags? If so, could be worth just adding version into this function. If not, then a separate function makes sense
|
Last thing: as specified in our OSPO Gen AI Policy that was just shipped, please fill out this information about Gen AI usage and add this to your PR description: AI Usage
If checked, please provide an explanation on how AI was used in the development of this pull request:
|
2 participants
Summary
This PR automates population of the
versionfield in generatedcode.jsonfiles so users no longer need to enter it manually.What Changed
GET /repos/{owner}/{repo}/releases/latest.code.jsonmetadata.Validation
src/__tests__/unit/helper.test.tssrc/__tests__/unit/main.test.tsNotes
code.jsonversion value so manual input remains available as a safe backup.