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

Add Support for Changed Spec Identification in spec-gen-sdk-runner tool #32763

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

raych1
Copy link
Member

@raych1 raych1 commented Feb 21, 2025

Major changes are to identify the changed specs to pass into the spec-gen-sdk tool for SDK generation.

  • [eng/pipelines/templates/stages/archetype-spec-gen-sdk.yml]
    Updated the parameters section to include Use1ESOfficial: false and increased the job timeout to 2400 minutes.

  • [eng/tools/spec-gen-sdk-runner/src/change-files.ts]
    Introduced a new function detectChangedSpecConfigFiles to detect changed specification configuration files in a pull request. This function identifies related readme.md and tspconfig.yaml files and logs the changes.

  • [eng/tools/spec-gen-sdk-runner/src/commands.ts]
    Added a new function generateSdkForSpecPr to generate SDKs for specification pull requests.

  • [eng/tools/spec-gen-sdk-runner/src/utils.ts]
    Added a new utility function resetGitRepo to reset unstaged changes in a git repository.

@weshaggard can you review these changes?

Copy link

openapi-pipeline-app bot commented Feb 21, 2025

Next Steps to Merge

Next steps that must be taken to merge this PR:
  • ❌ The required check named Protected Files has failed. Refer to the check in the PR's 'Checks' tab for details on how to fix it and consult the aka.ms/ci-fix guide

@raych1 raych1 self-assigned this Feb 21, 2025
Copy link

openapi-pipeline-app bot commented Feb 21, 2025

PR validation pipeline restarted successfully. If there is ApiView generated, it will be updated in this comment.

@raych1 raych1 changed the title Support changed spec identification in the spec-gen-sdk-runner tool Add Support for Changed Spec Identification in spec-gen-sdk-runner tool Feb 21, 2025
*/
export async function resetGitRepo(repoPath: string): Promise<void> {
try {
const { stderr } = await execAsync("git clean -fd && git reset --hard HEAD", {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
const { stderr } = await execAsync("git clean -fd && git reset --hard HEAD", {
const { stderr } = await execAsync("git clean -fdx && git reset --hard HEAD", {

Add x so ignored files also get deleted. Will reset HEAD be correct? It will only work if we haven't create a new branch and committed those changes, or do we just need a clean git status?

Copy link
Member Author

Choose a reason for hiding this comment

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

'reset' HEAD is needed to remove any modifications to the tracked files.

const workingFolder = ".";
const workPath = path.resolve(process.cwd(), workingFolder, options.specFolder, searchPath);
// Execute the git command using exec
const { stdout, stderr } = await execAsync(`git ls-tree ${options.treeId} ${workPath}`);
Copy link
Member

Choose a reason for hiding this comment

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

Why are we use a git command here instead of just traversing the file system? I would expect by the name of this function we are only looking at the files on disk.

logMessage("No files changed in the PR");
}
logMessage(`Changed files in the PR: ${prChangedFiles.length}`);
const fileList = prChangedFiles.filter((p) => !p.includes("/scenarios/"));
Copy link
Member

Choose a reason for hiding this comment

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

What is special about "scenarios" here?

Copy link
Member Author

Choose a reason for hiding this comment

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

We intend to exclude SDK generation for changes coming from the scenarios folder.

Copy link
Member

Choose a reason for hiding this comment

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

There are also other paths like examples should those be included? I'm mostly asking this question as it seems a little odd to only figure out this one folder type if what you are looking for is json files.

Copy link
Member Author

@raych1 raych1 Feb 21, 2025

Choose a reason for hiding this comment

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

Some other file types also need to be included, such as *.tsp, tspconfig.yaml, readme.md.
Changes in the examples folder would need to trigger the automation run because samples would be used in the SDK test/sample generation step. I am not aware of any other folders we might have in the specification folder. I will also add 'specification' to the filter.

const { stdout: headCommitRaw } = await execAsync("git rev-parse HEAD");
const headCommit = headCommitRaw.trim(); // Trim any newline characters
const { stdout: treeIdRaw } = await execAsync(`git rev-parse ${headCommit}^{tree}`);
const treeId = treeIdRaw.trim();
Copy link
Member

Choose a reason for hiding this comment

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

Why are we looking at git here? We should just be using the files on disk and assume that repo is correctly checked out.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: 🔬 Dev in PR
Development

Successfully merging this pull request may close these issues.

2 participants