-
Notifications
You must be signed in to change notification settings - Fork 722
Dotnet vscode csharp insertion #8563
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: dev/aajohn/relautomation
Are you sure you want to change the base?
Dotnet vscode csharp insertion #8563
Conversation
tasks/insertionTasks.ts
Outdated
const branchName = await createBranch(options.roslynEndSHA, options.roslynVersion); | ||
console.log(`Branch created: ${branchName}`); | ||
|
||
if (options.createPullRequest) { |
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 option necessary? When do we want to create the branch but not the pull request?
tasks/insertionTasks.ts
Outdated
|
||
// Step 2: Get current SHA from package | ||
const currentSHA = await getCurrentRoslynSHA(); | ||
options.roslynStartSHA = currentSHA || '0000000000000000000000000000000000000000'; |
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.
this should just throw if we can't find the current roslyn package sha
tasks/insertionTasks.ts
Outdated
|
||
interface InsertionOptions { | ||
roslynVersion?: string; | ||
roslynStartSHA?: string; |
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.
this doesn't seem to be passed in ever, probably can remove.
tasks/insertionTasks.ts
Outdated
); | ||
return stdout || '(no PRs with required labels)'; | ||
} catch (error) { | ||
console.warn('PR finder failed, using empty list:', error); |
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.
use the azdo log warn syntax, e.g.
vscode-csharp/tasks/createTagsTasks.ts
Line 190 in 4ca4cf4
function logWarning(message: string): void { |
tasks/insertionTasks.ts
Outdated
return null; | ||
} | ||
|
||
async function cloneRoslynRepo(): Promise<void> { |
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 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
- You can use a an existing clone of roslyn when running locally.
- You don't have to worry about cleaning up the repo here (pipeline automatically cleans it up).
tasks/insertionTasks.ts
Outdated
} | ||
|
||
const authJson = { | ||
GitHubToken: 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.
should this be using options.githubPAT
instead?
tasks/insertionTasks.ts
Outdated
const packageJson = JSON.parse(fs.readFileSync(packageJsonPath, 'utf8')); | ||
|
||
if (!packageJson.defaults) { | ||
packageJson.defaults = {}; |
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.
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'); |
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.
@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?
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.
It looks like I haven't updated pr-finder yet. Opened dotnet/roslyn-tools#1500
tasks/insertionTasks.ts
Outdated
let changelogContent = fs.readFileSync(changelogPath, 'utf8'); | ||
|
||
// Format PR list with proper indentation | ||
const formattedPRList = prList.split('\n').map(line => ` ${line}`).join('\n'); |
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.
do we always get these style new lines? Potentially should use node's 'os' EOL here since this is from the console output
tasks/insertionTasks.ts
Outdated
async function createBranch(endSHA: string, version: string): Promise<string> { | ||
console.log('Creating and pushing branch...'); | ||
|
||
await execAsync('git config user.name "Azure Pipelines"'); |
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.
Instead of doing all of this again, can we reuse the logic from
vscode-csharp/tasks/localizationTasks.ts
Line 79 in 4ca4cf4
const newBranchName = `localization/${parsedArgs.commitSha}`; |
Uh oh!
There was an error while loading. Please reload this page.