Add support for .azdignore #4146
Conversation
jongio
force-pushed
the
azdignore
branch
2 times, most recently
from
fd610e8
to
c5d9ddf
Compare
|
@wbreza - Please also see this update: Relevant code: |
|
@pamelafox - This allows template authors to specify which files should be ignored when a template consumer runs |
There was a problem hiding this comment.
The .azdignore stuff looks good and I like how we were able to leverage an existing package (and hence file format) that matches .gitignore semantics.
The support for other remote types gave me a bit of pause, just because of all the path mangling going on.
In practice what sort of files are going to be .azdignore'd? Are folks who clone a template that uses this technology setting themselves up for failure later when they create a repository based on the code they got from the template (minus the ignored stuff)?
| return path, nil | ||
| } | ||
|
|
||
| path = strings.TrimRight(path, "/") | ||
| // Check if path is a relative or absolute file path | ||
| if isRelativePath(path) || isAbsolutePath(path) { |
There was a problem hiding this comment.
This was all added so we could support azd init --template <some-local-filesystem-path-that-point-at-a-git-repo>?
I will admit I'm a little nervous about all the path munging here (especially with how we have to handle windows specific drive letter paths, but also even on nix some of the time)?
Is this related to the .azdignore stuff or just another nice to have while you were in the area? Is initializing from a template collocated on your machine a common scenario we want to support?
There was a problem hiding this comment.
While writing tests for .azdignore, I wanted to dynamically build templates with various files to test all patterns. Instead of creating physical templates for each scenario, I added the ability to reference a local template. This enhancement allowed me to test the end-to-end process using azd init, ensuring the files were copied correctly according to the .azdignore rules.
The tests now cover both Windows and POSIX environments, ensuring compatibility across different operating systems.
Do you have any suggestions for alternative approaches or improvements to this method?
There was a problem hiding this comment.
At a first glance, I wonder with this being a public feature, scoping as an isolated change with a more clear line-of-sight how the feature affects the system as a whole may be helpful.
I believe that the result of this change as-is, based on how this logic is embedded into templates path calculation, is that the upstream template source can now specify relative paths, or drive paths, and azd would gladly process that.
For example, if awesome-azd (or any remote template source) added a new template entry with the RepositoryPath set to a local relative path, I believe azd would honor it.
I tested this by doing the following:
azd template source add local --type file --location cli/azd/resources/templates.json- Added an entry:
{
"name": "Local template",
"description": "",
"repositoryPath": "../../../",
"tags": [
"bicep"
]
},
azd init- Chose Local template
azdgladly processed the relative path but would fail if the path isn't a valid git repository.
I don't think this is a huge deal by any means, but I feel that template sources just shouldn't be allowed to do this, but I'm happy to be convinced otherwise.
Overall, I do understand and buy-in to the need here for e2e testing purposes from an authoring perspective. I just wonder if it's more prudent to just add this type of local testing handling to the azd init --template <path> command directly to avoid.
| } | ||
|
|
||
| switch parsedURL.Scheme { | ||
| case "http", "https", "ssh", "git", "file": |
There was a problem hiding this comment.
We didn't support ssh:// before for templates, are we sure the right thing will happen with respect to auth or are folks going to get stuck being prompted for their passphrase or something if we try to git clone?
There was a problem hiding this comment.
I included ssh:// here because I wanted to be inclusive of all git schemes. I'm not stuck on including it. I don't mind pulling it for now if you think that is better.
This is the first step towards thinking about 2 personas "template authors" vs "template consumers". A template author would use .azdignore to exclude files they don't want to be used by a template consumer, such as specialized pipeline files, or other eng sys assets that the template consumer wouldn't need. |
|
Hm, so when I saw this PR title, I initially assumed that azdignore would describe files to get ignored by the deployment process. Instead, its describing files to get ignored by init, and doesnt seem to relate to deploy? I usually prefer that people do a git clone, not azd init, so that they can easily pull changes from upstream, so I don't have a huge opinion about the azd init flow. |
| func Absolute(path string) (string, error) { | ||
| // already a git URI, return as-is | ||
| if strings.HasPrefix(path, "git") || strings.HasPrefix(path, "http") { | ||
| path = strings.TrimRight(path, string(filepath.Separator)) |
There was a problem hiding this comment.
filepath.Separator is the OS-specific separator, on Windows this would be \. I believe with this change, Azure-Samples/todo-python-mongo/ (which we have seen happen on awesome-azd) no longer gets trimmed to Azure-Samples/todo-python-mongo on a Windows machine.
We are considering moving to a model where we have two personas "template authors" and "template consumers". We want all template consumers to use That way the template author can include things in the template that are needed for the template to say pass ci or some other validation that is needed for authoring the template. And the template consumer isn't going to need all of those files. So template author would use .azdignore file to exclude them. Do you have an example of deployment time azd should ignore that isn't already covered in a platform idiomatic way? .dockerignore etc. |
|
@jongio Two reasons for my interpretation of .azdignore -
As for your proposal: I guess "azd init" makes sense for templates where you're just getting infra and building on it. I'd still want developers to use "git clone" for repos like azure-search-openai-demo where they're actively pulling updates to a fully functional app. Also, if a dev opens in Codespaces and runs azd init, they'll still have all the original files, right? (Since the old ones dont get deleted, I think?) Or are you imagining a different Codespaces flow? |
|
@pamelafox - For the template author vs template consumer problem "don't give the consumer more files than they need" - do you think that is a real problem or are people okay with getting more files than they need? I will look into using azdignore for your other scenario. |
This feels more like service-host specific, @jongio. So, I would consider adding something unique for Azure WebApp (service host level), as using something like .azdignore for this might have no sense for ACA and/or SWA. |
|
@jongio Its certainly possible that extra files might be overwhelming or folks getting started with templates. In user studies, I think I only saw that once with a developer looking at the notebooks/ folder instead of the src/ folder in azure-search-openai-demo. Maybe devs are used to ignoring stuff like .github, tests, etc. |
jongio
force-pushed
the
azdignore
branch
2 times, most recently
from
62d8663
to
aef9154
Compare
|
@pamelafox once someone uses and template on starts making it their own we don't really want them working in a branch of the template repo do we? Are you thinking that its a fork and clone and they maintain the upstream relationship throughout the lifecycle of them product work? |
|
@pamelafox - I have added the ability to use .azdignore for Can you try this out and see if this is what you are looking for? Right now it defaults to removing the pycache and the venv folders. Are you saying there and some cases where you want azd to package those files/folders? |
|
Yes, we are thinking of disabling the Oryx build on App Service (for security reasons with our Vnet option), which means we'd need to send the whole environment with Python packages up to App Service. Not sure how you could support that in a backward compatible way.. Maybe if there's a .azdignore, it wouldnt default to ignoring anything? |
Here's what is excluded by default today. All:
Python:
Node:
Will update logic to: If user has .azdignore, then only ignore .azure by default. Don't ignore |
|
We just had a design discussion for this:
|
Azure Dev CLI Install InstructionsInstall scriptsMacOS/Linux
bash: pwsh: WindowsPowerShell install MSI install Standalone Binary
MSI
Documentationlearn.microsoft.com documentationtitle: Azure Developer CLI reference
|
|
.zipignore PR #4258 |
| ignoreMatcher, err := azdignore.ReadIgnoreFiles(staging) | ||
| if err != nil && !os.IsNotExist(err) { | ||
| return fmt.Errorf("reading .azdignore file: %w", err) | ||
| } | ||
|
|
||
| err = azdignore.RemoveIgnoredFiles(staging, ignoreMatcher) | ||
| if err != nil { | ||
| return fmt.Errorf("removing ignored files: %w", err) | ||
| } |
There was a problem hiding this comment.
Feels like the RemoveIgnoredFiles should internally determine the files to ignore from a file path instead of having to read them and pass them in.
| } | ||
|
|
||
| // RemoveIgnoredFiles removes files and directories based on .azdignore rules using a pre-collected list of paths. | ||
| func RemoveIgnoredFiles(staging string, ignoreMatchers []gitignore.GitIgnore) error { |
There was a problem hiding this comment.
nit: consider renaming this to stagingPath to be more clear that it is a file path.
| func RemoveIgnoredFiles(staging string, ignoreMatchers []gitignore.GitIgnore) error { | |
| func RemoveIgnoredFiles(stagingPath string, ignoreMatchers []gitignore.GitIgnore) error { |
| } | ||
|
|
||
| // ShouldIgnore checks if a file or directory should be ignored based on a slice of gitignore.GitIgnore structures. | ||
| func ShouldIgnore(path string, isDir bool, ignoreMatchers []gitignore.GitIgnore) bool { |
There was a problem hiding this comment.
Nit: Consider not exporting this function (make lowercase) if its only used internally within the package.
| } | ||
|
|
||
| // CollectFilePaths collects all file and directory paths under the given root directory. | ||
| func CollectFilePaths(root string) ([]string, error) { |
There was a problem hiding this comment.
nit: Same as previous comment about not exporting function.
| if filepath.Base(path) == ".azdignore" { | ||
| return true | ||
| } |
There was a problem hiding this comment.
Did we ever land on whether we were going to call this .zipignore or not? Or look into if app service has any existing behavior we could snap to?
| "github.com/stretchr/testify/require" | ||
| ) | ||
|
|
||
| func TestAbsolute(t *testing.T) { |
There was a problem hiding this comment.
nit: Test function naming.
| func TestAbsolute(t *testing.T) { | |
| func Test_Absolute(t *testing.T) { |
There was a problem hiding this comment.
Same for other unit tests function names.
| files := map[string]string{ | ||
| "azure.yaml": "azure content", | ||
| "infra/main.bicep": "main bicep content", | ||
| ".azdignore": `*.log | ||
| tmp/* | ||
| !tmp/important.txt | ||
| *.bak | ||
| nested/*/ | ||
| secret.yaml | ||
| exactfile.txt | ||
| /level1/level2/file.txt | ||
| *.hidden | ||
| /foo/* | ||
| !.gitignore | ||
| **/foo/bar | ||
| !/foo/bar.baz | ||
| abc/**/def | ||
| a?c.txt`, | ||
| "error.log": "log content", | ||
| "tmp/ignored.txt": "should be ignored", | ||
| "tmp/important.txt": "should not be ignored", | ||
| "backup.bak": "backup content", | ||
| "nested/dir1/ignored.file": "should be ignored", | ||
| "nested/dir2/ignored.file": "should be ignored", | ||
| "nested/dir3/important.txt": "should be ignored", | ||
| "secret.yaml": "secret content", | ||
| "exactfile.txt": "exact file match", | ||
| "level1/level2/file.txt": "specific path match", | ||
| "hidden.hidden": "hidden file match", | ||
| "foo/ignore.txt": "foo directory ignored", | ||
| "foo/bar": "foo/bar file ignored", | ||
| "foo/bar.baz": "foo/bar.baz file not ignored", | ||
| "abc/some/def/file.txt": "nested match with wildcards", | ||
| "acc.txt": "single character wildcard match", | ||
| "abc.txt": "single character wildcard match", | ||
| } |
There was a problem hiding this comment.
Consider moving this .azdignore sample to a file.
|
Is zip too specific and this is more of a ignore on deploy in azd terms? deployignore? Will we use a similar concept if the output is not in the form of a zip file in the future? Even just a lose folder of files that is ready to deploy? |
|
Focusing on .zipignore in other PR. Closing this for now. #4258 |
Fixes #4142
Summary
This pull request introduces support for
.azdignorefiles in theInitializercomponent, providing users with the ability to specify files and directories to be excluded during initialization. This functionality mirrors the behavior of.gitignorefiles, enhancing the flexibility and control over which files are included in the project setup. Additionally, the ability to load a template from a local folder has been added to facilitate ease of end-to-end testing with the init function.Scenario
A template author needs file in the repo to validate the repo, but doesn't want a consumer of the template to have those files copied when they call
azd init.With the addition of
.azdignoresupport, users can now define ignore patterns upfront, simplifying the initialization process.New Features
Support for
.azdignoreFiles:.azdignorefile in the root of the project directory..azdignorefile uses the same syntax as.gitignore, allowing users to specify patterns for files and directories to be excluded.Automatic Exclusion of Specified Files:
Initializercomponent will read the.azdignorefile and automatically exclude the specified files and directories from being copied to the project directory.Loading Templates from Local Folders:
How to Use
Create a
.azdignoreFile:.azdignore.Example
.azdignorefile:Initialize the Project:
.azdignorefile will be excluded automatically.Use Local Templates:
azd init --template ./path/to/local/templateTo-Do
.azdignorefiles.