Skip to content

Conversation

deepakrathore33
Copy link

@deepakrathore33 deepakrathore33 commented Aug 25, 2025

  • Using gulp tasks instead of bash scripts

@deepakrathore33 deepakrathore33 requested a review from a team as a code owner August 25, 2025 15:35
const branchName = await createBranch(options.roslynEndSHA, options.roslynVersion);
console.log(`Branch created: ${branchName}`);

if (options.createPullRequest) {
Copy link
Member

Choose a reason for hiding this comment

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

Is this option necessary? When do we want to create the branch but not the pull request?


// Step 2: Get current SHA from package
const currentSHA = await getCurrentRoslynSHA();
options.roslynStartSHA = currentSHA || '0000000000000000000000000000000000000000';
Copy link
Member

Choose a reason for hiding this comment

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

this should just throw if we can't find the current roslyn package sha


interface InsertionOptions {
roslynVersion?: string;
roslynStartSHA?: string;
Copy link
Member

Choose a reason for hiding this comment

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

this doesn't seem to be passed in ever, probably can remove.

);
return stdout || '(no PRs with required labels)';
} catch (error) {
console.warn('PR finder failed, using empty list:', error);
Copy link
Member

Choose a reason for hiding this comment

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

use the azdo log warn syntax, e.g.

function logWarning(message: string): void {

return null;
}

async function cloneRoslynRepo(): Promise<void> {
Copy link
Member

Choose a reason for hiding this comment

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

I think it'd be better to not clone the repo as part of the task - instead pass in the path to the repo as a parameter. And for the pipeline, clone the repo in the yaml.

That way

  1. You can use a an existing clone of roslyn when running locally.
  2. You don't have to worry about cleaning up the repo here (pipeline automatically cleans it up).

}

const authJson = {
GitHubToken: process.env.GITHUB_TOKEN || '',
Copy link
Member

Choose a reason for hiding this comment

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

should this be using options.githubPAT instead?

const packageJson = JSON.parse(fs.readFileSync(packageJsonPath, 'utf8'));

if (!packageJson.defaults) {
packageJson.defaults = {};
Copy link
Member

Choose a reason for hiding this comment

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

this should probably just throw if we couldn't get the defaults from the package.json


// Setup auth for roslyn-tools
const homeDir = process.env.HOME || process.env.USERPROFILE;
const settingsDir = path.join(homeDir!, '.roslyn-tools');
Copy link
Member

Choose a reason for hiding this comment

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

@JoeRobich I didn't see anything for it obviously, but is it possible to pass the github token directly (either by an env var or parameter), instead of writing to a settings file?

Copy link
Member

Choose a reason for hiding this comment

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

It looks like I haven't updated pr-finder yet. Opened dotnet/roslyn-tools#1500

let changelogContent = fs.readFileSync(changelogPath, 'utf8');

// Format PR list with proper indentation
const formattedPRList = prList.split('\n').map(line => ` ${line}`).join('\n');
Copy link
Member

Choose a reason for hiding this comment

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

do we always get these style new lines? Potentially should use node's 'os' EOL here since this is from the console output

async function createBranch(endSHA: string, version: string): Promise<string> {
console.log('Creating and pushing branch...');

await execAsync('git config user.name "Azure Pipelines"');
Copy link
Member

Choose a reason for hiding this comment

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

Instead of doing all of this again, can we reuse the logic from

const newBranchName = `localization/${parsedArgs.commitSha}`;
and extract out a helper that creates a branch and pull request from the local changes?

@deepakrathore33 deepakrathore33 marked this pull request as draft August 25, 2025 18:17
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.

3 participants