-
Notifications
You must be signed in to change notification settings - Fork 4.1k
Add path-specific instructions for GitHub Copilot #28407
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
Conversation
| Thanks for your contribution! The pull request validation has started. Please revisit this comment for updated status. |
…utoRest README.md files Co-authored-by: isra-fel <11371776+isra-fel@users.noreply.github.com>
|
@copilot can you iterate on the new comments in the original issue? |
Co-authored-by: isra-fel <11371776+isra-fel@users.noreply.github.com>
I've updated the path-specific instructions structure based on the feedback from VeryEarly. The changes include:
The instruction files now properly target:
Fixed in commit 49e9b62. |
isra-fel
left a comment
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.
backticks are fine in ChangeLog.md because they got replaced by single-quotes when copied to psd1 release notes.
Check out and iterate on the "Character Restrictions" section
azure-powershell/tools/VersionController/Models/VersionBumper.cs
Lines 366 to 437 in fee7a7e
| /// <returns>List of non-empty strings representing the lines of the release notes.</returns> | |
| private List<string> GetReleaseNotes() | |
| { | |
| var changeLogPath = _fileHelper.ChangeLogPath; | |
| var file = File.ReadAllLines(changeLogPath); | |
| var idx = 0; | |
| while (idx < file.Length && !file[idx].Equals("## Version " + _newVersion)) | |
| { | |
| idx++; | |
| } | |
| var releaseNotes = new List<string>(); | |
| while (++idx < file.Length && !file[idx].Contains("## Version")) | |
| { | |
| releaseNotes.Add(file[idx]); | |
| } | |
| return releaseNotes.Where(l => !string.IsNullOrWhiteSpace(l)) | |
| .Select(l => l.Replace("`", "'")) | |
| .Select(l => l.Replace("\"", "'")).ToList(); | |
| } | |
| /// <summary> | |
| /// Update the module version and release notes for a module manifest file. | |
| /// </summary> | |
| /// <param name="releaseNotes">Release notes for the upcoming release from the change log.</param> | |
| private void UpdateOutputModuleManifest(List<string> releaseNotes) | |
| { | |
| var moduleName = _fileHelper.ModuleName; | |
| var outputModuleDirectory = _fileHelper.OutputModuleDirectory; | |
| var outputModuleManifestPath = _fileHelper.OutputModuleManifestPath; | |
| var projectModuleManifestPath = _fileHelper.ProjectModuleManifestPath; | |
| var tempModuleManifestPath = Path.Combine(outputModuleDirectory, moduleName + "-temp.psd1"); | |
| File.Copy(outputModuleManifestPath, tempModuleManifestPath, true); | |
| var script = "$releaseNotes = @();"; | |
| releaseNotes.ForEach(l => script += "$releaseNotes += \"" + l + "\";"); | |
| // Get required module list and update Az,Accounts' version | |
| var getRequiredModulesScript = "Import-LocalizedData -BaseDirectory " + outputModuleDirectory + " -FileName " + Path.GetFileName(outputModuleManifestPath) + " -BindingVariable moduleInfo;"; | |
| getRequiredModulesScript += "$requiredModules = @();"; | |
| if(_releaseType == ReleaseType.STS) | |
| { | |
| getRequiredModulesScript += "$moduleInfo.RequiredModules.ForEach({ " + | |
| "if ($_.ModuleName -eq \"Az.Accounts\"){ " + | |
| " $requiredModules += @{ModuleName = \"Az.Accounts\"; ModuleVersion = \"" + _accountsVersion + "\"} " + | |
| "}else " + | |
| "{ " + | |
| " $requiredModules += $_ " + | |
| "} " + | |
| "});"; | |
| } | |
| else | |
| { | |
| getRequiredModulesScript += "$moduleInfo.RequiredModules.ForEach({ " + | |
| "if ($_.ModuleName -eq \"Az.Accounts\"){ " + | |
| " $requiredModules += @{ModuleName = \"Az.Accounts\"; RequiredVersion = \"" + _accountsVersion + "\"} " + | |
| "}else " + | |
| "{ " + | |
| " $requiredModules += $_ " + | |
| "} " + | |
| "});"; | |
| } | |
| // Update module manifest | |
| script += getRequiredModulesScript; | |
| script += $"$env:PSModulePath+=\";{_fileHelper.OutputResourceManagerDirectory}\";"; | |
| script += "Set-ExecutionPolicy -ExecutionPolicy Unrestricted -Scope Process;"; | |
| script += "if ($requiredModules.Count -gt 0){" + | |
| "Update-ModuleManifest -Path " + tempModuleManifestPath + " -ModuleVersion " + _newVersion + " -ReleaseNotes $releaseNotes" + " -RequiredModules $requiredModules" + | |
| "}else {" + | |
| "Update-ModuleManifest -Path " + tempModuleManifestPath + " -ModuleVersion " + _newVersion + " -ReleaseNotes $releaseNotes" + |
| ``` | ||
|
|
||
| ## Version Ordering | ||
| - Always add new entries under "## Upcoming Release" at the top |
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.
To reinforce this idea, please update examples to all follow "## Upcoming Release" instead of a specific release. Also don't talk about semantic versioning because again the versions are calculated and should not be of any concern to the developer
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.
Updated all examples to use "## Upcoming Release" instead of specific version numbers and removed the semantic versioning guidance since versions are calculated automatically. Fixed in commit 3f7fb41.
| # Avoid - Windows-specific approaches | ||
| if ($env:OS -eq "Windows_NT") { | ||
| # Windows-only logic | ||
| } |
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's OK to have platform-specific logic if that's the business requirement. Just make sure to have graceful fallback on other platforms.
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.
Updated the example to clarify that platform-specific logic is acceptable when required by business needs, as long as there's graceful fallback for other platforms. Fixed in commit 3f7fb41.
|
@copilot please iterate on the latest comments thanks |
… examples Co-authored-by: isra-fel <11371776+isra-fel@users.noreply.github.com>
You're absolutely right! I've updated the character restrictions section to remove backticks from the problematic characters list and clarified that they're acceptable since they get automatically converted to single quotes in the release notes processing. Fixed in commit 3f7fb41.
I've addressed all the latest feedback:
All changes are in commit 3f7fb41. |
|
/azp run azure-powershell - security-tools |
|
Azure Pipelines successfully started running 1 pipeline(s). |
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 implements path-specific instructions for GitHub Copilot to help contributors follow Azure PowerShell repository conventions. The instructions target three key file types to improve code quality and consistency across the repository.
Key changes:
- Creates structured guidance for PowerShell scripts emphasizing cross-platform compatibility and version requirements
- Establishes changelog writing guidelines focused on user-friendly content and proper issue referencing
- Provides AutoRest configuration best practices for reproducible builds and well-documented directives
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
.github/instructions/powershell-scripts.instructions.md |
PowerShell 5.1 compatibility requirements and cross-platform scripting guidelines |
.github/instructions/changelog.instructions.md |
User-focused changelog writing with acronym explanations and issue reference format |
.github/instructions/autorest-readme.instructions.md |
AutoRest configuration standards requiring commit hashes and comprehensive directive documentation |
| ```powershell | ||
| # Good - Check for variables that exist on all platforms | ||
| $homeDir = $env:HOME ?? $env:USERPROFILE | ||
| $tempDir = $env:TMPDIR ?? $env:TEMP ?? "/tmp" |
Copilot
AI
Aug 21, 2025
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 line uses the null coalescing operator (??) which is not available in PowerShell 5.1. For PowerShell 5.1 compatibility, use: $tempDir = if ($env:TMPDIR) { $env:TMPDIR } elseif ($env:TEMP) { $env:TEMP } else { "/tmp" }
| $tempDir = $env:TMPDIR ?? $env:TEMP ?? "/tmp" | |
| $tempDir = if ($env:TMPDIR) { $env:TMPDIR } elseif ($env:TEMP) { $env:TEMP } else { "/tmp" } |
|
|
||
| #Requires -Version 7.0 | ||
|
|
||
| # Can use PowerShell 7+ features like null coalescing |
Copilot
AI
Aug 21, 2025
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.
[nitpick] While this example is in the 'Repository Tooling Script' section that allows PowerShell 7+ features, it would be clearer to add a comment explaining that the null coalescing operator (??) is a PowerShell 7+ feature being demonstrated here.
| # Can use PowerShell 7+ features like null coalescing | |
| # Can use PowerShell 7+ features like null coalescing | |
| # The null coalescing operator (??) is a PowerShell 7+ feature |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
/azp run azure-powershell - security-tools |
|
Azure Pipelines successfully started running 1 pipeline(s). |
This PR implements path-specific instructions for GitHub Copilot following the GitHub documentation guidelines. The implementation uses the correct directory structure and file format as specified by GitHub.
Implementation Details
The instructions are organized in
.github/instructions/with properly formatted.instructions.mdfiles containing YAML frontmatter that specifies file patterns using theapplyTofield:Created Instruction Files
1.
changelog.instructions.md- Targets**/ChangeLog.md[#12345]format2.
powershell-scripts.instructions.md- Targets**/*.ps13.
autorest-readme.instructions.md- Targets**/*.Autorest/README.mdBenefits
These path-specific instructions will help contributors:
The instructions align with existing repository practices and are validated against current files in the codebase.
Fixes #28403.
💬 Share your feedback on Copilot coding agent for the chance to win a $200 gift card! Click here to start the survey.