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 the --pattern CLI argument for file system globbing #16456

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

Conversation

anthony-c-martin
Copy link
Member

@anthony-c-martin anthony-c-martin commented Feb 21, 2025

For the following CLI commands:

  • build
  • build-params
  • format
  • lint
  • restore

Adds the following optional argument:
--pattern <pattern>: Selects all files matching the specified glob pattern.

This is implemented using the .NET built-in file globbing support: https://learn.microsoft.com/en-us/dotnet/api/microsoft.extensions.filesystemglobbing

Closes #246
Closes #1285

Note that while various workarounds have been provided in #1285, using bash or powershell loops, they are very slow because of .NET cold-start delays.

Copy link
Contributor

github-actions bot commented Feb 21, 2025

Test this change out locally with the following install scripts (Action run 13534805277)

VSCode
  • Mac/Linux
    bash <(curl -Ls https://aka.ms/bicep/nightly-vsix.sh) --run-id 13534805277
  • Windows
    iex "& { $(irm https://aka.ms/bicep/nightly-vsix.ps1) } -RunId 13534805277"
Azure CLI
  • Mac/Linux
    bash <(curl -Ls https://aka.ms/bicep/nightly-cli.sh) --run-id 13534805277
  • Windows
    iex "& { $(irm https://aka.ms/bicep/nightly-cli.ps1) } -RunId 13534805277"

Copy link
Contributor

github-actions bot commented Feb 21, 2025

Dotnet Test Results

    78 files   -     39      78 suites   - 39   31m 6s ⏱️ - 22m 13s
11 894 tests +    54  11 894 ✅ +    54  0 💤 ±0  0 ❌ ±0 
27 546 runs   - 13 567  27 546 ✅  - 13 567  0 💤 ±0  0 ❌ ±0 

Results for commit 3d1d1b4. ± Comparison against base commit 70068c2.

This pull request removes 1796 and adds 683 tests. Note that renamed tests count towards both.

		nestedProp1: 1
		nestedProp2: 2
		prop1: true
		prop2: false
	1
	2
	\$'")
	prop1: true
	prop2: false
…
Bicep.Cli.IntegrationTests.BuildCommandTests ‑ Build_should_compile_files_matching_pattern (False)
Bicep.Cli.IntegrationTests.BuildCommandTests ‑ Build_should_compile_files_matching_pattern (True)
Bicep.Cli.IntegrationTests.BuildParamsCommandTests ‑ BuildParams_should_compile_files_matching_pattern (False)
Bicep.Cli.IntegrationTests.BuildParamsCommandTests ‑ BuildParams_should_compile_files_matching_pattern (True)
Bicep.Cli.IntegrationTests.FormatCommandTests ‑ Format_should_format_files_matching_pattern (False)
Bicep.Cli.IntegrationTests.FormatCommandTests ‑ Format_should_format_files_matching_pattern (True)
Bicep.Cli.IntegrationTests.LintCommandTests ‑ Lint_should_compile_files_matching_pattern (False)
Bicep.Cli.IntegrationTests.LintCommandTests ‑ Lint_should_compile_files_matching_pattern (True)
Bicep.Cli.IntegrationTests.RestoreCommandTests ‑ Restore_should_succeed_for_files_matching_pattern (False)
Bicep.Cli.IntegrationTests.RestoreCommandTests ‑ Restore_should_succeed_for_files_matching_pattern (True)
…

♻️ This comment has been updated with latest results.

@StephenWeatherford
Copy link
Contributor

StephenWeatherford commented Feb 21, 2025

Should we consider allowing globs such as "**/*.bicep" directly in the -f parameter?

I asked Copilot, and it could only find the following similar usage, --pattern (but note that it's not true glob patterns, and blob paths are not file paths):
https://learn.microsoft.com/en-us/cli/azure/storage/file/copy?view=azure-cli-latest
Related: Azure/azure-cli#10382

One possible ambiguity case is "file[1].cs". And supporting it with "-f" users would need to make sure to put pattern in double quotes.

Sounds like the explicit approach probably is safer.

@shenglol
Copy link
Contributor

shenglol commented Feb 21, 2025

Should we consider allowing globs such as "**/*.bicep" directly in the -f parameter?

I asked Copilot, and it could only find the following similar usage, --pattern (but note that it's not true glob patterns, and blob paths are not file paths): https://learn.microsoft.com/en-us/cli/azure/storage/file/copy?view=azure-cli-latest Related: Azure/azure-cli#10382

One possible ambiguity case is "file[1].cs". And supporting it with "-f" users would need to make sure to put pattern in double quotes.

Sounds like the explicit approach probably is safer.

It doesn't seem like Matcher supports

Should we consider allowing globs such as "**/*.bicep" directly in the -f parameter?

I asked Copilot, and it could only find the following similar usage, --pattern (but note that it's not true glob patterns, and blob paths are not file paths): https://learn.microsoft.com/en-us/cli/azure/storage/file/copy?view=azure-cli-latest Related: Azure/azure-cli#10382

One possible ambiguity case is "file[1].cs". And supporting it with "-f" users would need to make sure to put pattern in double quotes.

Sounds like the explicit approach probably is safer.

It looks like Matcher doesn't support ? or [] (character classes and ranges), so ambiguity shouldn't be an issue. Personally, I like the idea of reusing the path parameter, but we may need to ensure wildcard matching works with absolute paths and remove the --file-pattern-root parameter.

To handle both absolute and relative file glob patterns, I think we can follow these steps:

  1. Check if the path contains *:

    • If no, perform single file compilation.
    • If yes, continue to the next step.
  2. Normalize the file glob pattern to an absolute path
    Use Path.GetFullPath to convert the file glob pattern to an absolute path.

  3. Split the absolute path at the first segment containing *:

    • The first part becomes the search root directory.
    • The second part is the relative file glob pattern.
  4. Use the search root directory and glob pattern with Matcher.

@anthony-c-martin
Copy link
Member Author

anthony-c-martin commented Feb 24, 2025

@StephenWeatherford, @shenglol thanks for the feedback and detailed notes! I don't have a super strong opinion either way - will plan to bring this up in Bicep Discussions.

Option 1: Introduce new --file-pattern argument

bicep build --file-pattern '*.bicep'
  • Pro: No possibility of mixup with existing CLI consumers.
  • Con: CLI arguments become more complex.
  • Neutral: More complex matching logic is possible (TBD whether this is actually desired/useful though, hence "neutral").

Option 2: Reuse the current "file path" argument

bicep build '*.bicep'
  • Pro: Simpler CLI API. @shenglol's suggestion of fitting this with the Matcher API seems viable.
  • Con: May not be possible to handle some of the more unusual glob edge cases, which may cause confusion. E.g. if a user tries to specify a glob pattern which doesn't contain *, we may incorrectly interpret this as a file path.
  • Neutral: Need to carefully consider whether this'll have any breaking changes on existing consumers of the CLI API.
  • Neutral: Need to clearly define how this'll interact with arguments that traditionally only support a single file (e.g. --stdout, where the consumer is just going to expect a single compiled output). Should we just block this combination outright if we detect the caller is using globs?

Other notes:

  • Known programmatic CLI consumers to consider: Azure CLI, Azure PowerShell, MSBuild task

Notes from discussion on 2/24:

  1. Option 1 got the popular vote
  2. We're going to rename --file-pattern to just --pattern
  3. Rather than introducing a --pattern-root argument, we will detect this from the path using @shenglol's suggestion
  4. See if we can support --outdir and preserve directory structure by default
  5. Make sure the error experience is good if someone gets the wildcard syntax wrong (e.g. bicep build --pattern *.bicep)

@anthony-c-martin anthony-c-martin changed the title Support file system globbing in CLI commands Add the --pattern CLI argument for file system globbing Feb 25, 2025
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.

Add --all parameter to bicep build Running bicep build should compile everything in the directory
3 participants