-
Notifications
You must be signed in to change notification settings - Fork 341
Add .webappignore and .funcignore Support for Service Packaging
#4258
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
Changes from all commits
c4e6486
fbee0d5
336d4c6
3558db3
486f162
4579236
32f23f2
014ae0e
a323e42
26346cd
76c4f5f
898aeb4
da8cca6
43153f1
c4fbaeb
714fff1
26f4199
566f839
02876f7
3c25693
51cf6a7
a10080f
a08c003
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -55,4 +55,4 @@ | |
| "default": "auth login --use-device-code" | ||
| } | ||
| ] | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,44 @@ | ||
| // Copyright (c) Microsoft Corporation. All rights reserved. | ||
| // Licensed under the MIT License. | ||
|
|
||
| package dotignore | ||
|
|
||
| import ( | ||
| "fmt" | ||
| "os" | ||
| "path/filepath" | ||
|
|
||
| "github.com/denormal/go-gitignore" | ||
| ) | ||
|
|
||
| // ReadDotIgnoreFile reads the ignore file located at the root of the project directory. | ||
| // If the ignoreFileName is blank or the file is not found, it returns nil, nil. | ||
| func ReadDotIgnoreFile(projectDir string, ignoreFileName string) (gitignore.GitIgnore, error) { | ||
| // Return nil if the ignoreFileName is empty | ||
| if ignoreFileName == "" { | ||
| return nil, nil | ||
| } | ||
|
|
||
| ignoreFilePath := filepath.Join(projectDir, ignoreFileName) | ||
| if _, err := os.Stat(ignoreFilePath); os.IsNotExist(err) { | ||
| // Return nil if the ignore file does not exist | ||
| return nil, nil | ||
| } | ||
|
|
||
| ignoreMatcher, err := gitignore.NewFromFile(ignoreFilePath) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("error reading %s file at %s: %w", ignoreFileName, ignoreFilePath, err) | ||
| } | ||
|
|
||
| return ignoreMatcher, nil | ||
| } | ||
|
|
||
| // ShouldIgnore determines whether a file or directory should be ignored based on the provided ignore matcher. | ||
| func ShouldIgnore(relativePath string, isDir bool, ignoreMatcher gitignore.GitIgnore) bool { | ||
| if ignoreMatcher != nil { | ||
| if match := ignoreMatcher.Relative(relativePath, isDir); match != nil && match.Ignore() { | ||
| return true | ||
| } | ||
| } | ||
| return false | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -9,34 +9,36 @@ import ( | |
| "path/filepath" | ||
| "time" | ||
|
|
||
| "github.com/azure/azure-dev/cli/azd/pkg/dotignore" | ||
| "github.com/azure/azure-dev/cli/azd/pkg/rzip" | ||
| "github.com/otiai10/copy" | ||
| ) | ||
|
|
||
| // CreateDeployableZip creates a zip file of a folder, recursively. | ||
| // createDeployableZip creates a zip file of a folder, recursively. | ||
| // Returns the path to the created zip file or an error if it fails. | ||
| func createDeployableZip(projectName string, appName string, path string) (string, error) { | ||
| // TODO: should probably avoid picking up files that weren't meant to be deployed (ie, local .env files, etc..) | ||
| // Create the output zip file path | ||
| filePath := filepath.Join(os.TempDir(), fmt.Sprintf("%s-%s-azddeploy-%d.zip", projectName, appName, time.Now().Unix())) | ||
| zipFile, err := os.Create(filePath) | ||
| if err != nil { | ||
| return "", fmt.Errorf("failed when creating zip package to deploy %s: %w", appName, err) | ||
| } | ||
|
|
||
| if err := rzip.CreateFromDirectory(path, zipFile); err != nil { | ||
| // if we fail here just do our best to close things out and cleanup | ||
| // Zip the directory without any exclusions (they've already been handled in buildForZip) | ||
| err = rzip.CreateFromDirectory(path, zipFile) | ||
| if err != nil { | ||
| zipFile.Close() | ||
| os.Remove(zipFile.Name()) | ||
| return "", err | ||
| } | ||
|
|
||
| // Close the zip file and return the path | ||
| if err := zipFile.Close(); err != nil { | ||
| // may fail but, again, we'll do our best to cleanup here. | ||
| os.Remove(zipFile.Name()) | ||
| return "", err | ||
| } | ||
|
|
||
| return zipFile.Name(), nil | ||
| return filePath, nil | ||
| } | ||
|
|
||
| // excludeDirEntryCondition resolves when a file or directory should be considered or not as part of build, when build is a | ||
|
|
@@ -48,17 +50,43 @@ type buildForZipOptions struct { | |
| excludeConditions []excludeDirEntryCondition | ||
| } | ||
|
|
||
| // buildForZip is use by projects which build strategy is to only copy the source code into a folder which is later | ||
| // zipped for packaging. For example Python and Node framework languages. buildForZipOptions provides the specific | ||
| // details for each language which should not be ever copied. | ||
| func buildForZip(src, dst string, options buildForZipOptions) error { | ||
| // buildForZip is used by projects to prepare a directory for | ||
| // zipping, excluding files based on the ignore file and other conditions. | ||
| func buildForZip(src, dst string, options buildForZipOptions, serviceConfig *ServiceConfig) error { | ||
| // Lookup the appropriate ignore file name based on the service kind (Host) | ||
| ignoreFileName := GetIgnoreFileNameByKind(serviceConfig.Host) | ||
|
|
||
| // Read and honor the specified ignore file if it exists | ||
| ignoreMatcher, err := dotignore.ReadDotIgnoreFile(src, ignoreFileName) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Are we also thinking about supporting In my mind, I think azd would honor both ignore files. If we're doing this, I think we should consider implementing the correct recursion logic in
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @ellismg had implemented
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. .funcignore only honors files in the service root and in discussion with them and the app service team we decided to not honor recursive .ignore files.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
That is quite an interesting decision. From a dev perspective, one main reason why I took a closer look. It also turns out that
If we're already engaged in conversations with both app service and function teams, can we consider:
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @weikanglim - Any objections to moving forward with the current implementation as it matches what .funcignore does today and what they would like for .webappignore. This means a service zipping only recognizes the .ignore file that is in the root of that service folder and not any parent .ignore files.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @jongio Just checking -- where are we in resolving the difference between of behavior in interpretation for Aside from that:
Overall, I really do love the direction here. But I would love if we can have a simple clean landing of a minimal change -- there are currently 37 file changes in this PR alone. I'm more than happy to work with you to land a minimal change if we can document all the necessary requirements in a new issue referencing #1039 so we're all on the same page, and also to serve as official documentation for the feature.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. On dotignore and recursive directories: Thanks for sharing your thoughts on dotignore. The current non-recursive behavior is consistent with how .funcignore works today, and I believe it's important to avoid introducing more differences. I’d prefer to maintain this alignment to keep things predictable and straightforward for developers. On changes to rzip.go: The changes in rzip.go are required to handle symbolic links in node_modules or virtual environments. Since these components could now be included, special code is necessary to ensure correct behavior. Keeping these updates within this PR ensures everything works seamlessly together. On e2e tests and test data files: Our current e2e tests are designed to cover any issues arising from the zip changes, particularly around symbolic links. The test data may be heavier, but it provides comprehensive coverage that ensures we don’t miss any potential edge cases. On default behavior for .funcignore: Regarding the behavior of .funcignore, I’m in touch with the functions team to verify if there are additional files that should be ignored, with or without the existence of a .funcignore file. It's also important to note that any issues where .funcignore doesn’t behave like .gitignore should be resolved within the func implementation. I don't think we should introduce behavior here that diverges from the expected .gitignore syntax. Thank you for the offer, but I’m confident the current implementation is in line with our goals, and I don't feel any additional simplification is needed at this time. I appreciate your willingness to assist and your thoughtful feedback throughout this process
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've left a comment on rzip.go that left me with some more questions. I'd still push towards creating a separate issue for
Would we consider transferring all the existing documentation on the PR into an issue so that we can have the requirements discussion? I'm also more than happy to help here if needed. Overall, I do really want this to see these changes landed -- I'm just trying to make sure we're covering enough our bases from enough angles to make this a good user experience for our users.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Here are the list of default excludes for function service type: Those will need to be excluded before zipping for function target. We may also want to confirm with app service team if there are default excludes like appsettings.development.json |
||
| if err != nil && !os.IsNotExist(err) { | ||
| return fmt.Errorf("reading %s file: %w", ignoreFileName, err) | ||
| } | ||
|
|
||
| // Temporary array to build exclude conditions dynamically | ||
| tempExcludeConditions := []excludeDirEntryCondition{} | ||
|
|
||
| // If there's no .ignore file, add the provided excludeConditions | ||
| if ignoreMatcher == nil { | ||
| tempExcludeConditions = append(tempExcludeConditions, options.excludeConditions...) | ||
| } else { | ||
| // If there's a .ignore file, apply ignoreMatcher only | ||
| tempExcludeConditions = append(tempExcludeConditions, func(path string, file os.FileInfo) bool { | ||
| relativePath, err := filepath.Rel(src, path) | ||
| if err == nil && dotignore.ShouldIgnore(relativePath, file.IsDir(), ignoreMatcher) { | ||
| return true | ||
| } | ||
| return false | ||
| }) | ||
| } | ||
|
|
||
| // these exclude conditions applies to all projects | ||
| options.excludeConditions = append(options.excludeConditions, globalExcludeAzdFolder) | ||
| // Always append the global exclusions (e.g., .azure folder) | ||
| tempExcludeConditions = append(tempExcludeConditions, globalExcludeAzdFolder) | ||
|
|
||
| // Copy the source directory to the destination, applying the final exclude conditions | ||
| return copy.Copy(src, dst, copy.Options{ | ||
| Skip: func(srcInfo os.FileInfo, src, dest string) (bool, error) { | ||
| for _, checkExclude := range options.excludeConditions { | ||
| // Apply exclude conditions (either the default or the ignoreMatcher) | ||
| for _, checkExclude := range tempExcludeConditions { | ||
| if checkExclude(src, srcInfo) { | ||
| return true, nil | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -12,47 +12,77 @@ import ( | |
| "strings" | ||
| ) | ||
|
|
||
| // CreateFromDirectory compresses a directory into a zip file. | ||
| func CreateFromDirectory(source string, buf *os.File) error { | ||
| w := zip.NewWriter(buf) | ||
| defer w.Close() | ||
|
|
||
| // Walk through the source directory | ||
| err := filepath.WalkDir(source, func(path string, info fs.DirEntry, err error) error { | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| if info.IsDir() { | ||
| return nil | ||
| } | ||
| fileInfo, err := info.Info() | ||
| // Fetch file info (Lstat avoids following symlinks) | ||
| fileInfo, err := os.Lstat(path) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| header := &zip.FileHeader{ | ||
| Name: strings.Replace( | ||
| strings.TrimPrefix( | ||
| strings.TrimPrefix(path, source), | ||
| string(filepath.Separator)), "\\", "/", -1), | ||
| Modified: fileInfo.ModTime(), | ||
| Method: zip.Deflate, | ||
| // Skip symbolic links | ||
| if fileInfo.Mode()&os.ModeSymlink != 0 { | ||
|
Comment on lines
+32
to
+33
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm curious about the motivation here to skip copying of symbolic links entirely. In my mind, the "righ" thing from an azd user perspective is that This is also the default behavior of gnuutils: It's unclear what the rest of the other changes in this file is -- I assume it's handling empty directories in the zip? I know I've received a lot of pushback here -- but would we consider creating a separate issue and PR just to have this land more smoothly?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm actually thinking that maybe we want to keep venvs excluded by default. I don't think we have a good use case for including venvs in a zip folder. @pamelafox - Can you confirm this? And also do you think that we should include pycache by default if they have a .ignore file? I'm thinking it will be large and might not be needed in any scenario. |
||
| return nil | ||
| } | ||
|
|
||
| f, err := w.CreateHeader(header) | ||
| if err != nil { | ||
| return err | ||
| // Create relative path and normalize it for zip | ||
| relativePath := filepath.ToSlash(strings.TrimPrefix(strings.TrimPrefix(path, source), string(filepath.Separator))) | ||
|
|
||
| // Handle directories by adding a trailing slash | ||
| if fileInfo.IsDir() { | ||
| relativePath += "/" | ||
| } | ||
| in, err := os.Open(path) | ||
|
|
||
| // Create zip header from the file info | ||
| header, err := zip.FileInfoHeader(fileInfo) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| _, err = io.Copy(f, in) | ||
|
|
||
| header.Name = relativePath | ||
| header.Modified = fileInfo.ModTime() | ||
|
|
||
| // Compress files (leave directories uncompressed) | ||
| if !fileInfo.IsDir() { | ||
| header.Method = zip.Deflate | ||
| } | ||
|
|
||
| // Write the header to the zip | ||
| writer, err := w.CreateHeader(header) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| // Write the file's content if it's not a directory | ||
| if !fileInfo.IsDir() { | ||
| if err := writeFileToZip(writer, path); err != nil { | ||
| return err | ||
| } | ||
| } | ||
|
|
||
| return nil | ||
| }) | ||
|
|
||
| return err | ||
| } | ||
|
|
||
| // writeFileToZip writes the contents of a file to the zip writer. | ||
| func writeFileToZip(writer io.Writer, filePath string) error { | ||
| file, err := os.Open(filePath) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| defer file.Close() | ||
|
|
||
| return w.Close() | ||
| _, err = io.Copy(writer, file) | ||
| return err | ||
| } | ||
Uh oh!
There was an error while loading. Please reload this page.