-
Notifications
You must be signed in to change notification settings - Fork 257
Add .azdxignore support for azd x watch #6153
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: main
Are you sure you want to change the base?
Conversation
Co-authored-by: jongio <[email protected]>
Co-authored-by: jongio <[email protected]>
|
@copilot what other |
Currently, However, the If future |
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 adds .azdxignore file support to azd x watch to allow users to exclude unwanted files and directories (like node_modules/, build outputs) from watch monitoring, preventing unnecessary rebuilds.
Key Changes
- Added gitignore-style pattern matching support using the existing
go-gitignorelibrary - Applied ignore logic to both directory enumeration and file change event handling in the file watcher
- Added unit tests to verify watcher creation and ignore functionality
Reviewed Changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| cli/azd/pkg/watch/watch.go | Added .azdxignore loading and isIgnored() helper method; integrated ignore checking in event handler and watchRecursive() |
| cli/azd/extensions/microsoft.azd.extensions/internal/cmd/watch.go | Applied same .azdxignore pattern matching to extension's watch command |
| cli/azd/pkg/watch/watch_test.go | Added new unit tests for watcher creation with/without ignore file and pattern verification |
| cli/azd/.vscode/cspell.yaml | Added "azdxignore" to spell check dictionary |
| cli/azd/extensions/azure.coding-agent/go.sum | Automatic dependency checksum updates (indirect dependencies) |
| // Check if the file is ignored by .azdxignore | ||
| if fw.ignorer != nil { | ||
| // Check if this is a file or directory for gitignore matching | ||
| info, statErr := os.Stat(event.Name) | ||
| isDir := statErr == nil && info.IsDir() | ||
| if fw.isIgnored(event.Name, isDir) { | ||
| continue | ||
| } | ||
| } |
Copilot
AI
Nov 13, 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.
Double file stat: The code calls os.Stat(event.Name) here (line 111) and then again later (line 122) within the same event handler. The second call on line 122 checks the same file again to determine if it's a directory.
Recommendation: Reuse the info and isDir variables from line 111-112 for the check on line 122, eliminating the redundant os.Stat call. This improves performance by avoiding the duplicate system call.
| isIgnored := fw.ignorer.Absolute(nodeModulesPath, true) | ||
| require.NotNil(t, isIgnored, "node_modules should be ignored") |
Copilot
AI
Nov 13, 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.
Using Absolute() instead of Relative(): The test uses fw.ignorer.Absolute(nodeModulesPath, true) but the actual implementation in isIgnored() uses fw.ignorer.Relative(relPath, isDir). This inconsistency means the test is not actually testing the same code path used during runtime.
Recommendation: Change this line to use the isIgnored() method instead:
isIgnored := fw.isIgnored(nodeModulesPath, true)
require.True(t, isIgnored, "node_modules should be ignored")This ensures the test validates the actual behavior of the watcher.
| isIgnored := fw.ignorer.Absolute(nodeModulesPath, true) | |
| require.NotNil(t, isIgnored, "node_modules should be ignored") | |
| isIgnored := fw.isIgnored(nodeModulesPath, true) | |
| require.True(t, isIgnored, "node_modules should be ignored") |
|
|
||
| // Cancel the context to stop the watcher | ||
| cancel() | ||
| time.Sleep(100 * time.Millisecond) // Give time for cleanup |
Copilot
AI
Nov 13, 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] Hardcoded sleep for synchronization: The test uses time.Sleep(100 * time.Millisecond) to wait for cleanup, which makes tests slower and potentially flaky. This pattern is repeated in all three tests.
Recommendation: Consider using a more robust synchronization mechanism, such as:
- Having the watcher signal when it's fully stopped via a channel, or
- Using a sync.WaitGroup to track goroutine completion
This would make tests more reliable and potentially faster.
| // Get the relative path from the root directory | ||
| relPath, err := filepath.Rel(fw.rootDir, path) | ||
| if err != nil { | ||
| // If we can't get relative path, use the path as-is |
Copilot
AI
Nov 13, 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] Missing documentation for the error handling fallback: The comment on line 204 says "If we can't get relative path, use the path as-is" but this fallback behavior may not work correctly with gitignore pattern matching. Gitignore patterns are designed to work with relative paths from the repository root, not absolute paths.
Recommendation: Add a comment explaining what happens in this edge case, or consider logging a warning when this fallback is used, since it might lead to unexpected behavior where patterns don't match as intended.
| // If we can't get relative path, use the path as-is | |
| // WARNING: If we can't get the relative path, we use the absolute path as-is. | |
| // This may cause gitignore pattern matching to fail, since patterns are relative to the root directory. | |
| log.Printf("watch: warning: could not determine relative path from root '%s' to '%s': %v. Using absolute path for ignore matching, which may not work as intended.", fw.rootDir, path, err) |
| isDir := statErr == nil && info.IsDir() | ||
| // Get the relative path from the root directory | ||
| relPath, relErr := filepath.Rel(cwd, event.Name) | ||
| if relErr != nil { |
Copilot
AI
Nov 13, 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] Same error handling issue as in main implementation: When filepath.Rel() fails, the code falls back to using the absolute path (line 131) for gitignore matching. However, gitignore patterns are designed to work with relative paths, not absolute paths, which could lead to patterns not matching as expected.
Recommendation: Consider the same approach as the main implementation - either log a warning or add documentation explaining this edge case behavior.
| if relErr != nil { | |
| if relErr != nil { | |
| fmt.Fprintf(os.Stderr, "Warning: failed to compute relative path for gitignore matching (%v); using absolute path, which may cause ignore patterns to not match as expected.\n", relErr) |
azd x watchmonitors all files in a project directory, including unwanted artifacts likenode_modules/and build outputs, causing unnecessary rebuilds.Changes
.azdxignorefrom working directory if present, using existinggo-gitignorelibrarypkg/watch/watch.go: AddedisIgnored()helper, updated event handler andwatchRecursive()extensions/microsoft.azd.extensions/internal/cmd/watch.go: Applied same pattern matching logicUsage
Create
.azdxignorein project root with standard gitignore syntax:Patterns are matched using relative paths from the working directory. If the file doesn't exist, all existing default ignores (
.git/, etc.) still apply.Testing
Added unit tests for watcher creation with/without ignore file and pattern verification.
Original prompt
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.